Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Бабин Георгий #191

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

tripples25
Copy link

if (finalTypes.Contains(obj.GetType()))
var type = obj.GetType();

if (TypeSerializers.TryGetValue(obj.GetType(), out var serializer))

Choose a reason for hiding this comment

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

А почему тут не переменная type используется?

string s1 = printer.PrintToString(person);

string s2 = person.PrintToString();

Choose a reason for hiding this comment

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

Давай тут var везде проставим?

var person = new Person { Name = "Alex", Age = 19 };
var person = new Person {Name = "Alex", Age = 19, Parent = new Person {Name = "Andrew", Age = 90}};

var printer = ObjectPrinter.For<Person>()

Choose a reason for hiding this comment

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

На данный момент ObjectPrinter не иммутабельный. Если я создам secondPrinter = printer.Printing(...) с новыми правилами, то они применятся и к старому принтеру.

sb.AppendLine(type.Name);
foreach (var propertyInfo in type.GetProperties())
var sb = new StringBuilder(type.Name + Environment.NewLine);
foreach (var propertyOrField in type.GetFieldsAndProperties(BindingFlags.Instance | BindingFlags.Public).Where(x => !excludedMembers.Contains(x.UnderlyingMember) && !excludedTypes.Contains(x.DataType)))

Choose a reason for hiding this comment

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

Давай вот это вот большое in (...) вынесем в отдельную переменную? Как-то уж слишком страшно выглядит.

Copy link
Author

Choose a reason for hiding this comment

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

Выделил в метод в новом коммите)

PrintToString(propertyInfo.GetValue(obj),
nestingLevel + 1));
sb.Append(identation + propertyOrField.Name + " = " +
(MemberSerializers.TryGetValue(propertyOrField.UnderlyingMember, out var propertyOrFieldSerializer)

Choose a reason for hiding this comment

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

Давай конечную сериализацию вынесем тоже в отдельную переменную? Начиная с (MemberSerializers. ...

{
var memberInfo = ((MemberExpression) memberSelector.Body).Member;
excludedMembers.Add(memberInfo);
return Excluding<TMemberType>();

Choose a reason for hiding this comment

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

Сейчас логика работает так, что если я исключаю какое-то конкретное поле, то исключаются все поля этого типа тоже. Похоже на какую-то опечатку.
Но её можно было избежать, если бы дополнить тесты и реализовать там все сценарии использования. ;)

Comment on lines 21 to 22
internal readonly Dictionary<Type, Func<object, string>> TypeSerializers = new();
internal readonly Dictionary<MemberInfo, Func<object, string>> MemberSerializers = new();

Choose a reason for hiding this comment

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

Не очень хорошо, что можно получить доступ к этим полям вне класса.

@@ -8,20 +11,23 @@ public class ObjectPrinterAcceptanceTests
[Test]

Choose a reason for hiding this comment

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

Давай докинем сюда больше тестовых случаев, чтобы покрыть все сценарии.
Плюс, по-хорошему, надо занести бы это в отдельный проект.

@MrTimeChip
Copy link

Ну и жду рабочие коллекции)

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