Трофимов Никита #200
Трофимов Никита #200
Conversation
ObjectPrinting/ISerializer.cs
Outdated
| { | ||
| string PrintToString(TOwner obj); | ||
| PropertySetting<TOwner> SelectProperty<P>(Expression<Func<TOwner, P>> properties); | ||
| ISerializer<TOwner> ChangeProperty<T2>(Func<object, string> method); |
There was a problem hiding this comment.
Почему именно Т2, а не просто, скажем, Т?)
| // using System; |
There was a problem hiding this comment.
В целом, это уже можно убрать, оно было нужно тебе, а не для ревью. В будущих дз можешь тоже убирать легаси-комментарии.
| namespace ObjectPrintingTests; | ||
|
|
||
| [TestFixture] | ||
| public class Tests |
There was a problem hiding this comment.
- У класса не самое говорящее название
- В тестах нигде не прослеживается структура ААА
- Тесты по делу, но их маловато. В частности, не увидел сериализацию с коллекциями, циклическими ссылками (см.задание). Не увидел, что твой сериализатор печатает корректное состояние объекта, если объект был изменен. Ну и вложенные объекты ещё покажи, пж, куда без них.
| { | ||
| private Person person; | ||
|
|
||
| [SetUp] |
There was a problem hiding this comment.
Не стоит привыкать к использованию метода SetUp. Его использование скорее редкость, нежели стандартная практика. В твоем случае ничего не мешает прописать private Person person => new() или использовать какие-нибудь фабрики / TestDataBuilder / Object Mother.
|
|
||
| namespace ObjectPrintingTests; | ||
|
|
||
| [TestFixture] |
There was a problem hiding this comment.
nit: я в атрибутах TestFixture и Test стараюсь инициализировать свойство TestOf
| propertiesOptions[propertyInfo.Name].IsExcept) | ||
| continue; | ||
|
|
||
| if (propertiesOptions.ContainsKey(propertyInfo.Name) && |
ObjectPrinting/PropertySetting.cs
Outdated
| using System; | ||
| using System.Globalization; | ||
| using Microsoft.VisualBasic.FileIO; | ||
| using NUnit.Framework; |
ObjectPrinting/StringSetting.cs
Outdated
| { | ||
| public class StringSetting<T> : PropertySetting<T> | ||
| { | ||
| protected internal int MaxLength { protected set; get; } |
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net7.0</TargetFramework> | ||
| <ImplicitUsings>enable</ImplicitUsings> |
There was a problem hiding this comment.
Точно ли тут нужно использовать ImplicitUsings? Выглядит так, словно ты создал консольное приложение, после чего добавил туда библиотеки для тестирования. Можно и так, но погугли про ImplicitUsings и реши, надо ли оно тебе.
ObjectPrintingTests/Usings.cs
Outdated
| @@ -0,0 +1,3 @@ | |||
| global using NUnit.Framework; | |||
| global using ObjectPrintingTests; | |||
…ля словаря и массива, написал тесты. Сделал на полное решение
| return stringSetting; | ||
| } | ||
|
|
||
| public ISerializer<TOwner> ChangeProperty<T2>(Func<object, string> method) |
There was a problem hiding this comment.
Соблюдай конвенцию нейминга. Вылазят то T2, то Т, то Р.
Обычно пишут - <T1, T2>, либо <T, R> но под R подразумевается Result.
Ещё как вариант, у тебя тоже частично есть - <TOwner, TProperty>
Стоит привести к единообразию.
Проверь везде, пожалуйста.
| public ISerializer<TOwner> ChangeProperty<T2>(Func<object, string> method) | ||
| { | ||
| optionsTypes.Add(typeof(T2), method); | ||
| return this; |
There was a problem hiding this comment.
Перед return обычно оставляют пустую строку. Это необязательно, конечно, но желательно.
Проверь везде, пожалуйста.
| sb.Append(propertyInfo.Name + " = " + | ||
| optionsType.Invoke(variable)); | ||
| } | ||
| else if (variable is IList list) |
There was a problem hiding this comment.
А если потребуется сериализовать hashset? Тоже коллекция, но не IList. Вопрос к тому, что расширение поддерживаемых типов сериализации будет происходить за счет добавления if?
| } | ||
|
|
||
| [Test] | ||
| public void PrintToString_WithList() |
There was a problem hiding this comment.
Осталось ещё либо в тестах для Person, либо ещё где добавить внутрь объекта пару полей/свойств и пару коллекций. Просто для полноты тестов
| } | ||
|
|
||
| [Test] | ||
| public void PrintIsUnchanged() |
There was a problem hiding this comment.
В таком случае имя теста слишком завязано на реализацию твоей сериализации. Стоит дать более говорящее название того, что тест проверяет. Допустим, сериализация_не_меняет_объект - осталось только написать это с учетом конвенций.
| } | ||
|
|
||
| [Test] | ||
| public void ExclusionFromPropertySerialization() |
There was a problem hiding this comment.
Тут мы исключаем тип данных из всего объекта, а не отдельную пропертю. Стоит дать более говорящее имя.
| } | ||
|
|
||
| [Test] | ||
| public void ChangingSerializationProperty() |
There was a problem hiding this comment.
Тут, если не ошибаюсь, тоже идет речь об изменении всех string в объекте. Если да - стоит изменить имя теста. Если нет - тогда мне неясен смысл .ChangeProperty<string>(_ => "Egor")
| } | ||
|
|
||
| [Test] | ||
| public void TrimmedString() |
There was a problem hiding this comment.
Trim - обрезание пробелов в конце и начале строки. Твой сериализатор делает не это. Тест, соответственно, тоже проверяет не операцию Trim
| return this; | ||
| } | ||
|
|
||
| private string PrintToString(object obj, int id, string name = "") |
There was a problem hiding this comment.
Точно ли класс с постфиксом Config должен держать в себе реализацию сериализации?
| public void PrintIsUnchanged() | ||
| { | ||
| var printer = ObjectPrinter.For<Person>(); | ||
| var s = printer.PrintToString(person); |
There was a problem hiding this comment.
Почему s? Почему бы не result, например? Обычно тестируемый модуль называют sut - system under test. Так проще ориентироваться в тесте.
Вообще, ещё есть cut - class under system. Пишут так крайне редко (по крайней мере, исходя из моего опыта), но для полноты картинки пусть будет.
| { | ||
| for (var i = 0; i < list.Count; i++) | ||
| sb.Append("\n" + i + " = " + | ||
| PrintToString(list[i], id + 1, propertyInfo.Name)); |
There was a problem hiding this comment.
Сериализация коллекций это хорошо - но у тебя в коде в цикле прописана рекурсия. По-хорошему, надо этот момент тоже учитывать - либо позволять извне настраивать глубину рекурсии, либо хотя бы оборачивать это дело в своё исключение, когда крашнется StackOverflow. А крашнуться оно может потому что кто-то в сериализатор запихнет коллекцию на 100к элементов, каждый из которых не примитив, а сложный объект.
| { | ||
| public class PropertySetting<T> | ||
| { | ||
| protected internal bool IsExcept { get; } |
There was a problem hiding this comment.
IsExcept не самое лучшее название. Во-первых, Except не существительное. Во-вторых, Except несколько перекликается с Exception - не стоит кому-то давать повод для разночтения твоего кода. Попробуй привязаться к другому слову. Стоит повертеть на языке то, что ты делаешь с пропертей - пропускаешь, не учитываешь, игнорируешь и т.д.
| var printer = ObjectPrinter.For<Person>(); | ||
| var s = printer | ||
| .SelectProperty(x => x.Birthday) | ||
| .ChangeCulture(new CultureInfo("en-US")) |
There was a problem hiding this comment.
Можно ли применить культуру для всего сериализуемого объекта, не указывая отдельные проперти?
@pakapik