Глейзер Роман. Object Printer. Реализация на 1 балл#257
Глейзер Роман. Object Printer. Реализация на 1 балл#257RomanGleyzer wants to merge 16 commits intokontur-courses:masterfrom
Conversation
ObjectPrinting/PrintingConfig.cs
Outdated
| private string PrintToString(object obj, int nestingLevel, HashSet<object> visited, MemberInfo member) | ||
| { | ||
| if (obj == null) | ||
| return "null" + Environment.NewLine; |
There was a problem hiding this comment.
При null не поймем что это был за тип. Может стоит как-то печатать эту инфу тоже?
| return _printingConfig; | ||
| } | ||
|
|
||
| public PrintingConfig<TOwner> Using(CultureInfo culture) |
There was a problem hiding this comment.
Не очень понятно по названию, мы культуру устанавливаем для сериализации конкретного поля, или для сериализации вообще всех полей в объекте? Что если мне нужно будет чтобы одна поляшка печаталась с одной культурой, а другая с другой?
There was a problem hiding this comment.
Не очень понятно по названию, мы культуру устанавливаем для сериализации конкретного поля, или для сериализации вообще всех полей в объекте? Что если мне нужно будет чтобы одна поляшка печаталась с одной культурой, а другая с другой?
Для случая одно поле с одной культурой, а другое с другой, такая реализация не подойдет
Добавил поддержку культуры на уровне конкретного member и поменял название
ObjectPrinting/PrintingConfig.cs
Outdated
| ArgumentNullException.ThrowIfNull(memberSelector); | ||
|
|
||
| if (memberSelector.Body is not MemberExpression expression) | ||
| throw new ArgumentException(null, nameof(memberSelector)); |
There was a problem hiding this comment.
Странно выкидывать ArgumentException с null
ObjectPrinting/PrintingConfig.cs
Outdated
| return member switch | ||
| { | ||
| FieldInfo field => field.GetValue(obj), | ||
| PropertyInfo property => property.GetValue(obj), |
There was a problem hiding this comment.
А чего будет если property ридонли?
There was a problem hiding this comment.
А чего будет если property ридонли?
Насколько я понял, PropertyInfo.GetValue(obj) корректно работает с readonly свойством, если у свойства есть public get. А если не публичный, то тогда работать не будет.
В обновленной реализации, если свойство геттер null или не публичный, то GetSerializableMembers не вернет такое свойство
ObjectPrinting/PrintingConfig.cs
Outdated
| namespace ObjectPrinting | ||
| namespace ObjectPrinting; | ||
|
|
||
| public class PrintingConfig<TOwner> |
There was a problem hiding this comment.
Давай основные для основных классов сделаем еще интерфейсы, типа IPrintingConfig
Так можно будет легче переопределить конкретную реализацию при необходимости, в теъ же тестах например
| return _printingConfig; | ||
| } | ||
|
|
||
| public PrintingConfig<TOwner> TrimmedToLength(int maxLength) |
There was a problem hiding this comment.
Почему он возвращает PrintingConfig а не PropertyPrintingConfig?
Что если я захочу написать .Printing(p => p.Name).TrimmedToLength(5).Using(mySerializer)
There was a problem hiding this comment.
Почему он возвращает PrintingConfig а не PropertyPrintingConfig? Что если я захочу написать .Printing(p => p.Name).TrimmedToLength(5).Using(mySerializer)
Тоже поправил, сейчас возвращается IPropertyPrintingConfig
ObjectPrinting/PrintingConfig.cs
Outdated
| return "cyclic reference" + Environment.NewLine; | ||
|
|
||
| var indent = new string('\t', nestingLevel + 1); | ||
| var sb = new StringBuilder(); |
There was a problem hiding this comment.
Можем StringBuilder один раз создать а не каждый раз при вызове метода?
| public class PrintingConfig<TOwner> | ||
| private readonly HashSet<Type> _excludedTypes = []; | ||
| private readonly HashSet<MemberInfo> _excludedMembers = []; | ||
| private readonly Dictionary<Type, Func<object, string>> _typeSerializers = []; |
There was a problem hiding this comment.
Что по потокобезопасности? Если я попробую твой сериализатор сразу из нескольких потоков использовать (один будет менять конфигурацию, а другой печатать), не будет ли каких-то проблем?
There was a problem hiding this comment.
Что по потокобезопасности? Если я попробую твой сериализатор сразу из нескольких потоков использовать (один будет менять конфигурацию, а другой печатать), не будет ли каких-то проблем?
В данном случае PrintingConfig не потокобезопасен. В этой реализации высок риск гонок
В обновленной реализации синхронизировал доступ с помощью lock
|
|
||
| private static bool IsFinalType(Type type) | ||
| { | ||
| return type.IsPrimitive |
There was a problem hiding this comment.
Что будет происходить с Nullable типами? Например int? сможем напечатать?
There was a problem hiding this comment.
Что будет происходить с Nullable типами? Например int? сможем напечатать?
В этой реализации IsFinalType вернет false и сериализация пойдет как для сложного объекта
В обновленной реализации поправил это с помощью Nullable.GetUnderlyingType(type) и дальше делал проверки с помощью IsFinalType
| return; | ||
| } | ||
|
|
||
| if (!visited.Add(obj)) |
There was a problem hiding this comment.
Не нужно ли очищать visited после полного обхода ветки?
Может быть такой кейс. Допустим у меня есть сложный класс Organization, в нем 2 поля MainNumber, SecondNumber с типом PhoneNumber. Если я в 2 поля положу один и тот же объект, кажется твой код вернет cyclic reference, хотя циклической зависимости нет.
There was a problem hiding this comment.
Не нужно ли очищать visited после полного обхода ветки?
Может быть такой кейс. Допустим у меня есть сложный класс Organization, в нем 2 поля MainNumber, SecondNumber с типом PhoneNumber. Если я в 2 поля положу один и тот же объект, кажется твой код вернет cyclic reference, хотя циклической зависимости нет.
Согласен. В исправлении теперь добавляем объект в visited перед тем как зайти в его поля или свойства, и убираем его из visited в finally, когда закончили печать этого объекта
| }; | ||
| } | ||
|
|
||
| private static bool IsFinalType(Type type) |
There was a problem hiding this comment.
Enum не fnal type?
Да, тут IsFinalType не учитывает enum. Добавил type.IsEnum в критерии финального типа
| return this; | ||
| } | ||
|
|
||
| public IPropertyPrintingConfig<TOwner, TProp> TrimmedToLength(int maxLength) |
There was a problem hiding this comment.
Сюда может попасть maxLength отрицательная?
There was a problem hiding this comment.
Сюда может попасть maxLength отрицательная?
Если смоделировать ситуацию, что попала отрицательная maxLength, то при обрезании строки будет выброшено исключение из-за неверного диапазона. Поэтому теперь maxLength проверяется и запрещается значения < 0
ObjectPrinting/PrintingConfig.cs
Outdated
| return member switch | ||
| { | ||
| FieldInfo field => field.GetValue(obj), | ||
| PropertyInfo property => property.GetValue(obj), |
There was a problem hiding this comment.
А если геттер у проперти выкинет исключение? Оно обработается как-то?
There was a problem hiding this comment.
А если геттер у проперти выкинет исключение? Оно обработается как-то?
Тут исключение из property.GetValue(obj) пробросится наружу и оборвет сериализацию. Я посчитал нужным обернуть получение значения в try/catch и в случае ошибки возвращать строковый маркер, чтобы сериализация продолжалась
|
|
||
| var normalizedType = NormalizeType(type); | ||
|
|
||
| lock (_sync) |
There was a problem hiding this comment.
Лок норм, но можно было бы просто новый объект каждый раз возвращать и все.
@PeterMotorniy