Skip to content

TEST Reflector #7

Open
khusainovilas wants to merge 1 commit intomainfrom
test2
Open

TEST Reflector #7
khusainovilas wants to merge 1 commit intomainfrom
test2

Conversation

@khusainovilas
Copy link
Owner

No description provided.

Copy link

@yurii-litvinov yurii-litvinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не всё так просто, но окей. И +1 за архитектурность (хотя тут это оверинжиниринг).


namespace Reflector.Test;

public class SampleClass

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надо, чтобы даже в тестовом коде StyleCop не выдавал предупреждений, иначе за пачкой "ожидаемых" можно пропустить то, которое по делу. Заглушили бы или сделали как он просит (тут просто вынести в отдельный файл можно было).

{
Reflector.DiffClasses(typeof(SampleClass), typeof(SampleClass2));
}
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это лучше, чем ничего, но такой тест пустые реализации тоже пройдут

/// <summary>
/// Represents a structural model of class.
/// </summary>
public class ClassModel

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это какая-то DTO-шка судя по виду, можно было бы попробовать сделать record-ом

}
else if (param.ParameterType.IsByRef)
{
modifier = "ref";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Хотя в современном C# это не так актуально, можно было не поддерживать

modifier = "ref";
}

methodModel.Parameters.Add(new ParameterModel(param.Name, param.ParameterType.Name, modifier));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут даже компилятор ругается, надо было что-то сделать по идее

/// </summary>
/// <param name="a">The first type.</param>
/// <param name="b">The second type.</param>
public static void DiffClasses(Type a, Type b)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это тоже можно было в отдельный класс вынести, а тут только вызывать метод этого отдельного класса. Это явно другая функциональность — принцип единственности ответственности нарушен.

var generics = model.GenericParameters.Count > 0 ? "<" + string.Join(", ", model.GenericParameters) + ">" : string.Empty;
var baseClass = !string.IsNullOrEmpty(model.BaseClass) ? " : " + model.BaseClass : string.Empty;

code += indent + modifiers + " class " + model.Name + generics + baseClass + "\n";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот бы ещё реализуемые классом интерфейсы

}
}

code += indent + " " + methodMod + " " + method.ReturnTypeName + " " + method.Name + methodGenerics + "(" + parameters + ")\n";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReturnTypeName вроде как в модель попадает как есть, и это будет настоящее имя типа из mscorlib.dll, т.е. System.Int32 вместо int. И если с int оно пойдёт и так, то с void беда будет, он рефлексией видится как Void.

"char" => "'\\0'",
"string" => "string.Empty",
_ => "null",
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно было просто default возвращать всегда, хотя как у Вас — красивее выглядеть будет

"bool" => "false",
"char" => "'\\0'",
"string" => "string.Empty",
_ => "null",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nullability не одобрит, лучше null!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants