Зайцев Дмитрий #228

Emi1337-ops · 2024-12-11T19:15:32Z

dmitgaranin · 2024-12-12T06:23:10Z

ObjectPrinting/Extensions/ObjectExtensions.cs

@@ -0,0 +1,16 @@
+using System;


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

Содержание коммитов должно быть атомарным. Один коммит - одно логическое изменение. Сейчас у тебя на все решение, правки, тесты - лишь один коммит =(
Зачем это нужно? Чтобы по истории коммитов, понимать, что произошло. Или уметь ревертить только "плохие" части. Или уметь их черри-пикать. И тд

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

Аналогичные комментарии и про новый коммит Refactor Homewark Solution. В нем и перенеслись тесты в самостоятельный проект, и произошло разделение логики PrintingConfig'а (даже не просто разделение, а еще доработка), и чистка зависимостей и лишних файлов. Каждый из подэтапов вполе уместно смотрелся бы отдельным коммитом

Дальше
Имя коммита Fix mistakes не говорит о том, что произошло внутри. Какие-то ошибки исправлены. Стоит именовать в соответствии с непосредственными изменениями. Например, Remove obsolete files. А, там еще и часть другого - тогда его в отдельный коммит и назвать Update PrintToString_Correct_WhenCallAllMethods logic или как-то так

А вот коммит Add Solution for Nestin Property выглядит вполне хорошо, как раз оптимально соответствует рекомендациям

dmitgaranin · 2024-12-12T06:24:00Z

ObjectPrinting/Extensions/ObjectExtensions.cs

@@ -0,0 +1,16 @@
+using System;
+
+namespace ObjectPrinting.Extensions;


Также не выполнен пункт про меншн наставника

dmitgaranin · 2024-12-12T06:24:36Z

ObjectPrinting/Extensions/ObjectExtensions.cs

+
+namespace ObjectPrinting.Extensions;
+
+public static class ObjectExtensions


Не нашел использований. Возможно, они должны быть

Удалил класс

dmitgaranin · 2024-12-12T06:26:45Z

ObjectPrinting/ObjectPrinting.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <None Update="Tests\Snapshots\ObjectPrinterTests.PrintToString_Correct_WhenCallAllMethods.verified.txt">


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

dmitgaranin · 2024-12-12T06:28:08Z

ObjectPrinting/PrintingConfig.cs

-namespace ObjectPrinting
+namespace ObjectPrinting;
+
+public class PrintingConfig<TOwner>


В этом классе оказалось много ответственности (нарушается SRP): конфигурация объектов для сериализации, непосредственно алгоритм сериализации, а также работа с рефлексией

Разделил

dmitgaranin · 2024-12-12T06:34:12Z

ObjectPrinting/PrintingConfig.cs

+        }
+
+        var collectionOnString = new StringBuilder();
+        var indentation = new string('\t', nestingLevel + 1);


'\t'` дублируется, полезно вынести в константу

вынес в константу

dmitgaranin · 2024-12-12T06:38:35Z

ObjectPrinting/PrintingConfig.cs

+
+        foreach (var property in type.GetProperties())
+        {
+            if (excludedTypes.Contains(property.PropertyType) || excludedProperties.Contains(property.Name))


А как в данном случае отработает логика?
Есть объект

public class Person1 { public Guid Id { get; set; } public Person2 Parent { get; set; } } public class Person2 { public Guid Id { get; set; } }

И мы исключили из сериализации только Person1.Id

Будет ли продолжать сериализоваться Person2.Id ?
Если не будет, корректное ли это поведение?
Если не будет, есть ли какие-то другие места с похожими потенциальными проблемами?

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

Решил проблему, теперь информация по свойствах\полях хранится в PropertyInfo, так же добавил тест PrintToString_NotSkipProperty_WhenNestingClass, проверяющий логику при вложенных полях

dmitgaranin · 2024-12-12T06:41:31Z

ObjectPrinting/PrintingConfig.cs

-            return sb.ToString();
+
+            var serializedProperty = SerializeProperty(property, obj, nestingLevel + 1);
+            serializedString.Append(indentation + property.Name + " = " + serializedProperty);


Предлагаю использовать интерполяцию

Не нашел исправлений

dmitgaranin · 2024-12-12T06:44:02Z

...sts/Snapshots/ObjectPrinterTests.PrintToString_AlternativePropertySerialization.verified.txt

@@ -0,0 +1,9 @@
+Person
+	Id = 00000000-0000-0000-0000-000000000000


Давай хотя бы один пример, что работает и на не пустом гуиде

Добавил в ObjectPrinterTests.PrintToString_AlternativePropertySerialization.verified
и ObjectPrinterTests.PrintToString_Correct_WhenCallAllMethods.verified

dmitgaranin · 2024-12-12T06:45:21Z

...ctPrinting/Tests/Snapshots/ObjectPrinterTests.PrintToString_SerializeDictionary.verified.txt

@@ -0,0 +1,23 @@
+Dictionary`2:


Что такое Dictionary2? И KeyValuePair2? и List1`?
Выглядит нечитаемым особенно в контексте задачи по сериализации)

изменил

dmitgaranin · 2024-12-13T05:52:27Z

ObjectPrinting/PrintingConfig.cs


-namespace ObjectPrinting


Куда-то пропал namespace почти везде

dmitgaranin · 2024-12-13T05:54:45Z

ObjectPrinting/ObjectSerializer.cs

+{
+    private readonly PrintingConfigStorage _config;
+    private readonly HashSet<object> _processedObjects = new();
+    private const char tab = '\t';


Для констант есть гайдлайны именования. Райдер их, кстати, подсказывает)

dmitgaranin · 2024-12-13T05:55:57Z

ObjectPrinting/ObjectSerializer.cs

+    public string Serialize(TOwner obj)
+    {
+        _processedObjects.Clear();
+        return SerializeObject(obj, 0);


Потенциальное NRE

Решить можно по разному, но хорошим решением было бы использовать MaybeNullWhen
Например так
private bool TrySerializeFinalType(object obj, [MaybeNullWhen(false)] out string serializedFinalType)

Рекомендую ознакомиться, очень полезный атрибут)
https://learn.microsoft.com/ru-ru/dotnet/api/system.diagnostics.codeanalysis.maybenullwhenattribute?view=net-8.0

dmitgaranin · 2024-12-13T06:00:08Z

ObjectPrinting/ObjectSerializer.cs

+                : obj.ToString() + Environment.NewLine;
+        }
+
+        if (_processedObjects.Contains(obj))


Стоит уделять внимание подсказкам от IDE, они могут помочь писать код чище)
Например здесь

Потому что сигнатура на добавление

dmitgaranin · 2024-12-13T06:00:53Z

ObjectPrinting/ObjectSerializer.cs

+
+    private bool TrySerializeFinalType(object obj, out string? serializedFinalType)
+    {
+        var type = obj.GetType();


Получение типа дублируется

dmitgaranin · 2024-12-13T06:20:53Z

fluent-api.sln

-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Spectacle", "Samples\Spectacle\Spectacle.csproj", "{EFA9335C-411B-4597-B0B6-5438D1AE04C3}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Spectacle", "Samples\Spectacle\Spectacle.csproj", "{EFA9335C-411B-4597-B0B6-5438D1AE04C3}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Tests", "Tests\Tests.csproj", "{0A6EEC4E-959A-4378-A8A9-B965F5F7D7FB}"


Скорее не просто Tests, а что-то вроде ObjectPrinter.Tests, по аналогии с проектом тестов в этом же солюшене

dmitgaranin · 2024-12-13T06:23:29Z

Tests/ObjectPrinterTests.cs

+            person,
+            new()
+            {
+                Id = new Guid("11111122-1234-1234-1234-000000000000"),


Для случайного гуида принято (и действительно проще) использовать Guid.NewGuid()

dmitgaranin · 2024-12-13T06:24:18Z

Tests/ObjectPrinterTests.cs

+        public Task PrintToString_SerializeDictionary()
+        {
+            var persons = new Dictionary<Person, string>
+        {


Что-то в этом файле совсем плохо с отступами и форматированием
Попробуй в райдере комбинацию ctrl + alt + l

dmitgaranin · 2024-12-13T06:52:07Z

Tests/ObjectPrinterTests.cs

@@ -0,0 +1,235 @@
+using System;
+using System.Collections.Generic;


Лишние usings

dmitgaranin · 2024-12-13T06:58:43Z

ObjectPrinting/Extensions/ObjectExtensions.cs

@@ -0,0 +1,16 @@
+using System;


Аналогичные комментарии и про новый коммит Refactor Homewark Solution. В нем и перенеслись тесты в самостоятельный проект, и произошло разделение логики PrintingConfig'а (даже не просто разделение, а еще доработка), и чистка зависимостей и лишних файлов. Каждый из подэтапов вполе уместно смотрелся бы отдельным коммитом

Дальше
Имя коммита Fix mistakes не говорит о том, что произошло внутри. Какие-то ошибки исправлены. Стоит именовать в соответствии с непосредственными изменениями. Например, Remove obsolete files. А, там еще и часть другого - тогда его в отдельный коммит и назвать Update PrintToString_Correct_WhenCallAllMethods logic или как-то так

А вот коммит Add Solution for Nestin Property выглядит вполне хорошо, как раз оптимально соответствует рекомендациям

Add Homework Task Solution

7b477f0

dmitgaranin reviewed Dec 12, 2024

View reviewed changes

Emi1337-ops added 3 commits December 12, 2024 18:54

Refactor Homewark Solution

6df1ebc

Fix mistakes

d447451

Add Solution for Nestin Property

5607fcd

dmitgaranin reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Зайцев Дмитрий #228

Зайцев Дмитрий #228

Emi1337-ops commented Dec 11, 2024 •

edited

Loading

dmitgaranin Dec 12, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 12, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 12, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024 •

edited

Loading

dmitgaranin Dec 12, 2024

Emi1337-ops Dec 12, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

dmitgaranin Dec 13, 2024

		@@ -0,0 +1,16 @@
		using System;

		namespace ObjectPrinting.Extensions;


		namespace ObjectPrinting.Extensions;

		public static class ObjectExtensions

		@@ -0,0 +1,9 @@
		Person
		Id = 00000000-0000-0000-0000-000000000000

		@@ -0,0 +1,235 @@
		using System;
		using System.Collections.Generic;

Зайцев Дмитрий #228

Are you sure you want to change the base?

Зайцев Дмитрий #228

Conversation

Emi1337-ops commented Dec 11, 2024 • edited Loading

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

Choose a reason for hiding this comment

Emi1337-ops Dec 12, 2024 • edited Loading

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

Emi1337-ops commented Dec 11, 2024 •

edited

Loading

Emi1337-ops Dec 12, 2024 •

edited

Loading