Шевырин Никита #225

alexadralt · 2024-12-11T16:56:03Z

GarageManager

Код хорошо декомпозирован, в целом сходу понятно что и зачем нужно, и круто, что сразу использовал библиотеку VerifyNUnit.

Но мне не хватило тестов, чтобы доверия к коду было больше. Не хватило теста на комбинацию разных видов форматирования в одном классе. Так же не хватило тестов на комбинацию коллекций, точно ли массив словарей будет хорошо сериализоваться? А если элементы коллекций - классы, а не примитивы? И в целом тесты есть только на явные кейсы из условия, но нет покрытия всяких краевых сценариев по типу тех, что я описал до этого. И еще сейчас у тебя есть одна фикстура на сразу для всего, это плохо, потому что у тебя есть разные сценарии и разные классы, которые хочется покрыть и выделить в отдельные отдельные классы в том числе и в тестах.

В целом создалось ощущение, что тесты написаны "для галочки", но тесты - настолько же важная часть разработки, как и написание основного кода программы. Хочется, чтобы ты тоже подумал над тем, какие есть сценарии, краевые случаи и т.п. и покрыл код качественнее и более осознанно.

GarageManager · 2024-12-11T19:06:48Z

ObjectPrinting/PrintingConfig.cs

+        internal CultureInfo DoubleCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo FloatCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo DateTimeCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal int MaxStringLength { get; set; } = int.MaxValue;


Сейчас ты ограничиваешь длину сразу для всех типов, но что, если мы захотим ограничить длину у одного конкретного поля? Сейчас твоя реализация не позволит такого сделать

Так же мне не кажется, что задание значения по умолчанию является удачным выбором. Число действительно большое, но это ограничение, которого без указания пользователем по идее быть не должно, в теории на вход может прийти строка, длина которой превосходит int.MaxValue, и тогда мы без предупреждения обрежем строку. Если пользователь явно не указал, то обрезать строку не надо

Вообще, если сослаться на исходник класса String в .NET, то там можно увидеть строку internal const int MaxLength = 0x3FFFFFDF;, в которой указано максимально возможная длина строки, и это значение сильно меньше чем int.MaxValue. Также если попытаться создать строку длины, большей, чем MaxLength, например так: var str = new string('a', 0x3FFFFFDF + 1);. То код падает с System.OutOfMemoryException

Да, ты прав, у строки действительно не может быть длины, превышающей это значение. Но мне все равно не кажется хорошей идея задавать потолок в поле, без явной на то необходимости. Как мне кажется, лучше было бы сделать поле нулабельным, и если значение на ограничение не устанавлено, то не обрезать строку вовсе

GarageManager · 2024-12-11T19:08:56Z

ObjectPrinting/PrintingConfig.cs

+        internal CultureInfo FloatCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo DateTimeCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal int MaxStringLength { get; set; } = int.MaxValue;
+        internal int MaxRecursionDepth { get; set; } = 16;


Если глубина рекурсии справедлива для всего PrintingConfig, то давай ее передавать в конструкторе, а не через метод

Поле можно сделать приватным

А не будет ли передача глубины рекурсии через конструктор противоречить идее Fluent api? У меня PrintingConfig вообще добывается через ObjectPrinter.For<T>()

Если в ObjectPrinter.For добавить необязательный параметр, который потом передавать в конструктор PrintingConfig'а, то противоречий не будет

GarageManager · 2024-12-11T19:14:28Z

ObjectPrinting/TypeSerializerExtensions.cs

+        return config;
+    }
+
+    public static PrintingConfig<TOwner> UseCulture<TOwner>(


В текущей реализации, если мы захотим указывать культуру для нового типа, то придется писать новый метод, и добавлять новое поле.
Если зайти в реализацию указанных тобой типов (float, double, DateTime), то можно найти кое-то, что их объединяет, не не только их, но и другие типы данных, поддерживающих форматирование с указанием культуры. Подумай, чем может тебе помочь это и то, что в сигнатуре UseCulture присутствует часть с дженериком <TOwner>

GarageManager · 2024-12-11T19:16:57Z

ObjectPrinting/PrintingConfig.cs

@@ -23,19 +96,84 @@ private string PrintToString(object obj, int nestingLevel)
                typeof(DateTime), typeof(TimeSpan)


Насколько эффективно каждый раз создавать эту коллекцию при условии, что она всегда одна и та же?

GarageManager · 2024-12-11T19:21:40Z

ObjectPrinting/PrintingConfig.cs

@@ -23,19 +96,84 @@ private string PrintToString(object obj, int nestingLevel)
                typeof(DateTime), typeof(TimeSpan)
            };
            if (finalTypes.Contains(obj.GetType()))


А если я захочу сериализовать, допустим, char или байт? Или любой другой примитив? Возможно для такого есть подходящий метод, покрывающий недостающие сценарии

GarageManager · 2024-12-11T19:29:07Z

ObjectPrinting/PrintingConfig.cs

+            if (obj is DateTime dateTimeValue)
+                return dateTimeValue.ToString(DateTimeCultureInfo) + Environment.NewLine;
+
+            return obj + Environment.NewLine;


Сейчас ты каждый раз добавляешь перевод строки, возможно получится как-то избежать этого повторения?

И еще ты сейчас дважды создаешь строку при конкатенации, т.е. сначала приводишь объект к строке, а потом создаешь новую с символом переноса строки, что неэффективно.

Подсказка: дальше по коду у тебя идет использование StringBuilder

GarageManager · 2024-12-11T19:30:16Z

ObjectPrinting/PrintingConfig.cs

+        {
+            if (obj is string stringValue)
+                return string.Concat(
+                    stringValue.AsSpan(0, Math.Min(MaxStringLength, stringValue.Length)),


Можешь подсказать, почему именно AsSpan, а не Substring?

Потому что SubString создаёт промежуточную строку, к которой мне потом надо приписать Environment.NewLine. За счёт AsSpan можно создавать на одну строку меньше.

GarageManager · 2024-12-11T19:36:33Z

ObjectPrinting/PrintingConfig.cs

+            return valueString;
+        }
+
+        private bool TryConvert(PropertyInfo propertyInfo, object? propertyValue, out string value)


Кажется, что если заинлайнить этот метод, то код станет проще и чище

GarageManager · 2024-12-11T19:37:32Z

ObjectPrinting/Tests/ObjectPrinterAcceptanceTests.CollectionSupport_Arrays.verified.txt

@@ -0,0 +1,8 @@
+Double[]:


Давай сериализовать коллекции (в особенности словари) в более приятную для чтения структуру? Например, сделать сериализацию приближенную к Json

alexadralt · 2024-12-12T19:34:39Z

@GarageManager

GarageManager

Оставил несколько минорных комментариев, в остальном все хорошо, задача зачтена

GarageManager · 2024-12-13T13:20:09Z

ObjectPrinting/PrintingConfig.cs

+            if (obj is string str)
+                return str.Substring(0, Math.Min(MaxStringLength, str.Length));
+
+            if (obj.GetType().IsValueType)


Я не совсем это имел в виду, когда говорил про создание одной и той же коллекции при вызове PrintToString. Ее можно было оставить, просто вынести в статическое поле класса

GarageManager · 2024-12-13T13:20:27Z

ObjectPrinting/PrintingConfig.cs

+        internal CultureInfo FloatCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo DateTimeCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal int MaxStringLength { get; set; } = int.MaxValue;
+        internal int MaxRecursionDepth { get; set; } = 16;


Если в ObjectPrinter.For добавить необязательный параметр, который потом передавать в конструктор PrintingConfig'а, то противоречий не будет

GarageManager · 2024-12-13T13:22:31Z

ObjectPrinting/PrintingConfig.cs

+        internal CultureInfo DoubleCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo FloatCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal CultureInfo DateTimeCultureInfo { get; set; } = CultureInfo.CurrentCulture;
+        internal int MaxStringLength { get; set; } = int.MaxValue;


Да, ты прав, у строки действительно не может быть длины, превышающей это значение. Но мне все равно не кажется хорошей идея задавать потолок в поле, без явной на то необходимости. Как мне кажется, лучше было бы сделать поле нулабельным, и если значение на ограничение не устанавлено, то не обрезать строку вовсе

alexadralt added 14 commits December 8, 2024 18:29

stuff done in class

441f072

added max string length configuration

bdadc67

renamed a variable and changed Use() method signature

0a315e1

added ForProperty() and ExceptProperty()

9605d4a

refactored ForType<>() related stuff

54a9708

implemented basic functionality

360534d

implemented culture support

12332f2

fixed a bug with MaxStringLength

9e2a950

added object extension methods for printing

c7b2f79

added support for collections

02918de

fix for recursion depth

c87c057

added tests

92dbb2a

added bounds check for WithMaxRecursionDepth()

d6aeaa8

refactored PrintToString()

50334f7

GarageManager suggested changes Dec 11, 2024

View reviewed changes

alexadralt added 7 commits December 12, 2024 14:44

improved culture support

743b6a8

made trimming individual string fields possible

69eca2a

added bounds check for maxLength parameter

90176f4

improved collections support and overall serialization style

d79b36a

updated basic tests

436cdd4

improved collections serialization

3c8a091

addded more tests

aea11ea

GarageManager approved these changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Шевырин Никита #225

Шевырин Никита #225

alexadralt commented Dec 11, 2024

GarageManager left a comment

GarageManager Dec 11, 2024

alexadralt Dec 12, 2024

GarageManager Dec 13, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

alexadralt Dec 12, 2024

GarageManager Dec 13, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

alexadralt Dec 12, 2024

GarageManager Dec 11, 2024

GarageManager Dec 11, 2024

alexadralt commented Dec 12, 2024

GarageManager left a comment

GarageManager Dec 13, 2024

GarageManager Dec 13, 2024

GarageManager Dec 13, 2024

		@@ -23,19 +96,84 @@ private string PrintToString(object obj, int nestingLevel)
		typeof(DateTime), typeof(TimeSpan)

Шевырин Никита #225

Are you sure you want to change the base?

Шевырин Никита #225

Conversation

alexadralt commented Dec 11, 2024

GarageManager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexadralt commented Dec 12, 2024

GarageManager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment