-
Notifications
You must be signed in to change notification settings - Fork 245
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
Шестопалов Андрей #219
base: master
Are you sure you want to change the base?
Шестопалов Андрей #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ревью
ObjectPrinting/PrintingConfig.cs
Outdated
sb.AppendLine($"{collectionType}:"); | ||
foreach (var element in enumerable) | ||
{ | ||
sb.Append(indentation + "- " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут лучше интерполяцию строк использовать.
PrintingConfig<TOwner> IPropertyPrintingConfig<TOwner, TPropType>.ParentConfig => printingConfig; | ||
} | ||
|
||
public interface IPropertyPrintingConfig<TOwner, TPropType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Давай интерфейс вынесем в отдельный файл
{ | ||
public class PrintingConfig<TOwner> | ||
private readonly List<Type> excludingTypes = []; | ||
private readonly Dictionary<Type, Func<object, string>> typeSerializers = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Давай по смыслу сгруппируем поля. typeSerializers с propertySerializers например, и excludingTypes с excludingProperties
ObjectPrinting/PrintingConfig.cs
Outdated
|
||
private static string HandleNull() | ||
{ | ||
return "null" + Environment.NewLine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У нас тут какая-то общая логика вырисовывается (добавление общей строки для каждого типа). Можем это вынести куда-то?
ObjectPrinting/PrintingConfig.cs
Outdated
return ((IFormattable)obj).ToString(null, culture) + Environment.NewLine; | ||
} | ||
|
||
private static bool IsFinalType(Type type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Получается каждый раз, когда метод вызывается будем новый массив со всеми типами создавать? Можно ли это сделать один раз где-то?
typeSerializers[typeof(TPropType)] = obj => serializer((TPropType)obj); | ||
} | ||
|
||
public void AddPropertySerializer(string propertyName, Func<object, string> serializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Получается для каждого типа будем свой отдельный метод добавлять? Можно завести отдельно TypeSerializer для каждого типа, и отдельные реализации для каждого поддерживаемого типа. Тогда получим несколько небольших классов, и у каждого будет своя понятная и небольшая зона ответственности. Кажется тогда этот класс будет не таким раздутым
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне кажется это не очень хорошая идея, ведь реализация сериализации задаётся функцией Func<TPropType, string>, а значит нам нужно хранить эту функцию и использовать в нужном месте. Не очень понимаю, как хранение этой функции в объекте поможет упростить наш класс. Мы ведь только добавим накладные ресурсы на создание каждого объекта, вместо того, чтобы сразу хранить функцию в словаре.
Кроме того, я постарался максимально упростить класс ObjectPrinting :)
ObjectPrinting/PrintingConfig.cs
Outdated
private string HandleDictionary(IDictionary dictionary, int nestingLevel, HashSet<object> visitedObjects) | ||
{ | ||
var dictionaryType = dictionary.GetType().Name; | ||
var indentation = new string('\t', nestingLevel + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можем работу с отступами куда нибудь в одно место вынести? А то кажется код дулируется тут
ObjectPrinting/PrintingConfig.cs
Outdated
return sb.ToString(); | ||
} | ||
|
||
private string HandleEnumerable(IEnumerable enumerable, int nestingLevel, HashSet<object> visitedObjects) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Как-будто сереализацию разных типов коллекций тоже можно вынести в отдельные классы или методы расширения
var propertyName = propertyInfo.Name; | ||
var propertyValue = propertyInfo.GetValue(obj); | ||
|
||
if (propertySerializers.TryGetValue(propertyName, out var propertySerializer)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У тебя в каких то файлах однострочные ифы с фигурных скобках, а где-то без. Давай одного стиля везде придерживаться (какого именно не очень важно)
No description provided.