Сахабутдинов Ильфир #211

ILFirV-V · 2024-12-09T05:30:30Z

dmitgaranin · 2024-12-09T14:16:47Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

@@ -0,0 +1,239 @@
+using System.Globalization;
+using FluentAssertions;


Привет) давай сначала общие рекомендации:

Содержание коммитов должно быть атомарным. Один коммит - одно логическое изменение. Например, в коммите Add. Решение 4 домашней работы добавлено вообще все: и первое задание, и второе, и тесты, и создание проектов под тест. Аналогично и во втором коммите - что такое рефакторинг? Без чтения кода мне неочевидно, какие были произведены изменения)
Предлагаю обсудить, какие есть плюсы и минусы у такого подхода

Прямо сейчас что-либо делать с коммитами не надо. Это рекомендации на будущее

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

Лучше несколько маленьких коммитов, чем один большой. Спасибо за рекомендацию

dmitgaranin · 2024-12-09T14:17:15Z

ObjectPrinting.Tests/ObjectPrinting.Tests.csproj

+    </PropertyGroup>
+
+    <ItemGroup>
+        <PackageReference Include="coverlet.collector" Version="6.0.0"/>


Правда в этом проекте используются все подключенные зависимости?

Не все. Поправил

dmitgaranin · 2024-12-09T14:18:17Z

ObjectPrinting/Configurations/PropertyPrintingConfig.cs

+
+    public PrintingConfig<TOwner> Using(Func<TPropType, string> print)
+    {
+        ArgumentNullException.ThrowIfNull(print);


Правда ли такая проверка нужна?

Да, это проверка нужна чтобы отлавливать некорректные случаи при конфигурировании и не падать во время сериализации (после изменений в коде, до этого свойство было nullable и были проверки в других местах, можно было вместо этой проверки игнорировать null и не выбрасывать исключение)

А наличия <Nullable>enable</Nullable> не достаточно?

Думаю достаточно. Поправил

dmitgaranin · 2024-12-09T14:19:04Z

ObjectPrinting/Configurations/PropertyPrintingConfig.cs

+        ArgumentNullException.ThrowIfNull(print);
+        Serializer = printObject =>
+        {
+            if (printObject is not TPropType propertyObject)


Предлагаю писать строго типизированный код. Т.е. писать так, чтобы не требовалась подобная проверка

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

Обычно, если в коде требуются проверки на корректность типа, то это жирный маркер, что что-то не так с архитектурой приложения. Вполне возможно, что все корректно и так действительно нужно, но примерно в 90% ситуаций это можно поправить переосмыслением подхода

Поправил

dmitgaranin · 2024-12-09T14:19:50Z

ObjectPrinting/Configurations/PropertyPrintingConfig.cs

+
+namespace ObjectPrinting.Configurations;
+
+public class PropertyPrintingConfig<TOwner, TPropType>(PrintingConfig<TOwner> printingConfig)


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

Мы можем добавить красивый Extension метод только для строки благодаря этому классу. Интерфейсы нужны для хранения настроек конфигурации в коллекции

dmitgaranin · 2024-12-09T14:34:51Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

+                .Using(x => $"{x} years")
+                .PrintToString(testObject);
+
+            printedString.Should().Contain($"{testObject.TestInt} years");


Тест не проверяет, что объект корректно сериализовался. Он лишь проверяет, что в тексте есть некоторая строка 10 years

Переписал с использованием Verify.NUnit

dmitgaranin · 2024-12-09T14:36:40Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

+                .UsingCommonCulture(culture)
+                .PrintToString(testObject);
+
+            printedString.Should().ContainAll(testDouble.ToString(culture), testFloat.ToString(culture));


Тест проверяет, что после сериализации есть некоторые строки которые как-то сконвертированы. Хочется проверить на конкретный ожидаемый результат

Переписал с использованием Verify.NUnit

dmitgaranin · 2024-12-09T14:37:44Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

+            var printedString = ObjectPrinter.For<PrintingTestObject>()
+                .PrintToString(testObject);
+
+            printedString.Should().Contain("circular reference");


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

Переписал с использованием Verify.NUnit

dmitgaranin · 2024-12-09T14:38:19Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

+            var printedString = ObjectPrinter.For<PrintingTestObject>()
+                .PrintToString(testObject);
+
+            printedString.Should().ContainAll("Item1", "Item2", "Item3");


Тест не проверяет сериализацию массива с отступами. Он лишь проверяет наличие трех строк в результате

Переписал с использованием Verify.NUnit

dmitgaranin · 2024-12-09T14:42:56Z

ObjectPrinting.Tests/ObjectPrinterAcceptanceTests.cs

+using ObjectPrinting.Extensions;
+using ObjectPrinting.Tests.TestObjects;
+
+namespace ObjectPrinting.Tests


Тесты должны проверять всю строку целиком, а не ее части

Нет теста на одновременный набор всех требований. Вдруг все вместе они не работают корректно?

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

Переписал с использованием Verify.NUnit, добавил тестов

…рации

…особа сериализации

Ilfir Sakhabutdinov added 2 commits December 8, 2024 20:06

Add. Решение 4 домашней работы

87fc4eb

Update. Рефакторинг кода

c690fba

dmitgaranin reviewed Dec 9, 2024

View reviewed changes

Ilfir Sakhabutdinov added 9 commits December 11, 2024 02:25

Fix. Обновление тестов по полученным замечаниям

0a5c42b

Fix. Исправление небольших замечаний

1adc983

Update. Изменения тестов дял разделение логики сериализации и конфигу…

08a843a

…рации

Fix. Разделение логики сериализации и конфигурации

c8fe4bf

Fix. Разделение настроек культуры и сериализации

9b8b71b

Fix. Вернул необходимые проверки перед добавлением альтернативного сп…

abc037e

…особа сериализации

Update. Выделение логики сериализации в отдельный класс

c32e92a

Update. Изменение тестов перед изменением способа настроек сериализации

a80092e

Fix. Изменение способа конфигурирования перед сериализацией

f37b491

		@@ -0,0 +1,239 @@
		using System.Globalization;
		using FluentAssertions;


		namespace ObjectPrinting.Configurations;

		public class PropertyPrintingConfig<TOwner, TPropType>(PrintingConfig<TOwner> printingConfig)

Сахабутдинов Ильфир #211

Are you sure you want to change the base?

Сахабутдинов Ильфир #211

Uh oh!

Conversation

ILFirV-V commented Dec 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ILFirV-V Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ILFirV-V Dec 11, 2024 •

edited

Loading