Conversation
…принтом и форматированием цикла
…оту с принтами отдельных обьектов
| return this; | ||
| } | ||
|
|
||
| public PrintingConfig<TOwner> SetNumericCulture(CultureInfo culture) |
There was a problem hiding this comment.
Уточнение Numeric кажется лишним. Всё таки это не только на числа влияет, но и на даты и тд
| return this; | ||
| } | ||
|
|
||
| public PrintingConfig<TOwner> Serialize<TType>(Func<TType, string> serializer) |
There was a problem hiding this comment.
Кажется, что решение недостаточно гибкое, из-за того что конфигурация делается в одном классе.
Как в твоём решении мне использовать одну культуру для дат, а другую для чисел?
Как мне понять что метод Trim работает только для строк (не заглядывая в твои исходники)?
Чтобы это порешать, возможно стоит выделить какие-то дополнительные сущности.
В идеале хочется чего-то такого:
ObjectPrinter.For<T>()
.For<int>().SetCulture().Serialize() // Здесь Trim не виден, тк тип int
.For(e => e.StringProp).Trim()
Также, так ты сразу видишь всю цепочку настройки конкретного пропа/типа
| Expression<Func<TOwner, TProperty>> property, Func<TProperty, string> serializer) | ||
| { | ||
| var body = property.Body as MemberExpression; | ||
| var propertyInfo = body?.Member as PropertyInfo; |
There was a problem hiding this comment.
Почему бы сразу не кидать ошибку, если конфигурация неправильная?
| { | ||
| private readonly List<IPrintStrategy> strategies = strategies.ToList(); | ||
|
|
||
| public string Print(object? obj, int nestingLevel, HashSet<object> visited) |
There was a problem hiding this comment.
Зачем метод принимает int nestingLevel, HashSet<object> visitedесли они всегда одинаковые?
There was a problem hiding this comment.
nestingLevel и visited передаются в метод, потому что при рекурсивной печати объекта они меняются на каждом шаге
| public class CycleFormatterStrategy : IPrintStrategy | ||
| { | ||
| private readonly Dictionary<object, int> visited = new(); | ||
| public bool CanHandle(Type type) => true; |
There was a problem hiding this comment.
Точно ли он должен обрабатывать все типы? Тут большой вопрос к производительности:
- Ты делаешь boxing не ссылочных типов https://learn.microsoft.com/ru-ru/dotnet/csharp/programming-guide/types/boxing-and-unboxing
- Ты записываешь
всёв hashset. Даже типы которые по факту не могут быть ссылочными
| IPropertyRenderer propertyRenderer) | ||
| { | ||
| var type = obj.GetType(); | ||
| var stringBuilder = new StringBuilder(); |
There was a problem hiding this comment.
Создавать StringBuilder на каждый объект накладно
| var current = propertyValue; | ||
| string? resultString = null; | ||
|
|
||
| foreach (var rule in rules.Where(r => r.CanApply(propertyInfo))) |
There was a problem hiding this comment.
Сигнатуры не сошлись. Здесь PropertyInfo? propertyInfo. В CanApply - PropertyInfo propertyInfo
Потенциальный NRE
|
|
||
| public RuleOutcome Apply(object value) | ||
| { | ||
| if (value is T typedValue) |
There was a problem hiding this comment.
Ты по сути это и проверяешь выше. Возможно стоит объединить методы, чтобы не делать двойную работу
There was a problem hiding this comment.
Проверка сверху проверяет возможность редактирования, условно, поля класса, но если мы захотим сделать такой запрос: For<Person>().Serialize<int>(x => $"I{x}").Serialize(x => x.Age, v => $"A{v}");
То у нас проверится правило для инта, оно пройдет CanApply, после проверится x.Age, что тоже проходит правило условие CanApply, но, т.к. мы на вызове Apply во второй раз, т.е. у x.Age кинется ошибка, т.к. у нас x.Age уже будет переопределен на стрингу, а в самом Rule останется функция на int Func<T, string> serializer. Поэтому это доп. проверка, что если переопределили тип, тогда никак не меняем.
| var outcome = ruleProcessor.ApplyRule(obj, propInfo); | ||
| if (outcome.Action == RuleResult.Skip) | ||
| { | ||
| return "null"; |
There was a problem hiding this comment.
Раз мы пропускаем поле, то его вообще не стоит выводить
|
|
||
| foreach (var prop in type.GetProperties(BindingFlags.Public | BindingFlags.Instance)) | ||
| { | ||
| var propLine = propertyRenderer.RenderProperty( |
There was a problem hiding this comment.
кажется, что PropertyRenderer усложняет код. Почему его работу нельзя доверить вызову recursivePrinter?
There was a problem hiding this comment.
я придерживался принципа srp, код так стал более гибким и в дальнейшем этот PropertyRender можно будет переиспользовать без проблем. у него одна ответственность. а recursivePrinter занимается выводом объекта. если объединить, то мы потеряем гибгость и в дальнейшем не будет ясно почему метод propertyrender лежит в recursivePrint
| { | ||
| private readonly List<IPrintStrategy> strategies = strategies.ToList(); | ||
|
|
||
| public string Print(object? obj, int nestingLevel, HashSet<object> visited) |
| return this; | ||
| } | ||
|
|
||
| public PropertyPrintingConfig<TOwner, string> Trim(int length) |
There was a problem hiding this comment.
Я могу вызвать For для !string поля и задать ему Trim. Хоть код и упадёт, но это совсем не User-friendly.
Чтобы это решить нужно использовать Extension-методы для Trim, где указать тип PropertyPrintingConfig<TOwner, string>
| return this; | ||
| } | ||
|
|
||
| public PrintingConfig<TOwner> Apply() => parent; |
There was a problem hiding this comment.
Можно было этого избежать добавив сюда методы из PrintingConfig.
Так ты бы разгрузил цепочку конфигурации и не вызывал лишний метод на каждый For
2 participants
@FnaTikJK