Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ObjectPrinting/Extensions/ObjectExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using System;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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


namespace ObjectPrinting.Extensions;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public static class ObjectExtensions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Удалил класс

{
public static string PrintToString<T>(this T obj)
{
return ObjectPrinter.For<T>().PrintToString(obj);
}

public static string PrintToString<T>(this T obj, Func<PrintingConfig<T>, PrintingConfig<T>> config)
{
return config(ObjectPrinter.For<T>()).PrintToString(obj);
}
}
18 changes: 18 additions & 0 deletions ObjectPrinting/Extensions/PropertyPrintingConfigExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
using System;

namespace ObjectPrinting.Extensions;

public static class PropertyPrintingConfigExtensions
{
public static PrintingConfig<TOwner> TrimmedToLength<TOwner>(
this PropertyPrintingConfig<TOwner, string> propConfig,
int maxLen)
{
if (string.IsNullOrEmpty(propConfig.PropertyName))
throw new ArgumentException("The name of the property is not specified.");

propConfig.ParentConfig.AddLengthProperty(propConfig.PropertyName, maxLen);

return propConfig.ParentConfig;
}
}
11 changes: 5 additions & 6 deletions ObjectPrinting/ObjectPrinter.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
namespace ObjectPrinting
namespace ObjectPrinting;

public static class ObjectPrinter
{
public class ObjectPrinter
public static PrintingConfig<T> For<T>()
{
public static PrintingConfig<T> For<T>()
{
return new PrintingConfig<T>();
}
return new PrintingConfig<T>();
}
}
9 changes: 9 additions & 0 deletions ObjectPrinting/ObjectPrinting.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,17 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="8.0.0-alpha.1" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тоже лишний пакет

<PackageReference Include="NUnit" Version="4.2.2" />
<PackageReference Include="NUnit3TestAdapter" Version="4.6.0" />
<PackageReference Include="Verify.NUnit" Version="28.4.0" />
</ItemGroup>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрал

<ParentFile>$([System.String]::Copy('%(FileName)').Split('.')[0].Split('(')[0])</ParentFile>
<DependentUpon>%(ParentFile).cs</DependentUpon>
</None>
</ItemGroup>
</Project>
226 changes: 200 additions & 26 deletions ObjectPrinting/PrintingConfig.cs
Original file line number Diff line number Diff line change
@@ -1,41 +1,215 @@
using System;
using System.Linq;
using System.Collections;
using System.Collections.Generic;
using System.Globalization;
using System.Linq.Expressions;
using System.Reflection;
using System.Text;

namespace ObjectPrinting

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

namespace ObjectPrinting;

public class PrintingConfig<TOwner>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Разделил

{
public class PrintingConfig<TOwner>
private readonly HashSet<Type> finalTypes =
[
typeof(string), typeof(DateTime), typeof(TimeSpan), typeof(Guid)
];
private readonly HashSet<Type> excludedTypes = [];
private readonly Dictionary<Type, Func<object, string>> typeSerializationMethods = [];
private readonly Dictionary<Type, IFormatProvider> typeCultures = [];
private readonly Dictionary<string, IFormatProvider> propertyCultures = [];
private readonly Dictionary<string, Func<object, string>> propertySerializationMethods = [];
private readonly HashSet<string> excludedProperties = [];
private readonly HashSet<object> processedObjects = [];
private readonly Dictionary<string, int> propertyLengths = new();

public PropertyPrintingConfig<TOwner, TPropType> Printing<TPropType>()
{
return new PropertyPrintingConfig<TOwner, TPropType>(this);
}

public PropertyPrintingConfig<TOwner, TPropType> Printing<TPropType>(Expression<Func<TOwner, TPropType>> memberSelector)
{
var propertyName = GetPropertyName(memberSelector);

return new PropertyPrintingConfig<TOwner, TPropType>(this, propertyName);
}

public string PrintToString(TOwner obj)
{
return PrintToString(obj, 0);
}

private string PrintToString(object? obj, int nestingLevel)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
public string PrintToString(TOwner obj)
if (obj == null)
{
return PrintToString(obj, 0);
return "null" + Environment.NewLine;
}

private string PrintToString(object obj, int nestingLevel)
if (processedObjects.Contains(obj))
{
//TODO apply configurations
if (obj == null)
return "null" + Environment.NewLine;
return "Circular Reference" + Environment.NewLine;
}

var finalTypes = new[]
{
typeof(int), typeof(double), typeof(float), typeof(string),
typeof(DateTime), typeof(TimeSpan)
};
if (finalTypes.Contains(obj.GetType()))
return obj + Environment.NewLine;

var identation = new string('\t', nestingLevel + 1);
var sb = new StringBuilder();
var type = obj.GetType();
sb.AppendLine(type.Name);
foreach (var propertyInfo in type.GetProperties())
if (TrySerializeFinalType(obj, out var serializedFinalType))
{
return serializedFinalType!;
}

if (TrySerializeCollection(obj, nestingLevel, out var serializedCollection))
{
return serializedCollection!;
}

return SerializeComplexType(obj, nestingLevel);
}

public PrintingConfig<TOwner> Excluding<TPropType>()
{
excludedTypes.Add(typeof(TPropType));
return this;
}

public PrintingConfig<TOwner> Excluding<TPropType>(Expression<Func<TOwner, TPropType>> memberSelector)
{
var propertyName = GetPropertyName(memberSelector);
excludedProperties.Add(propertyName);
return this;
}

public void SpecifyTheCulture<TType>(CultureInfo culture)
{
typeCultures[typeof(TType)] = culture;
}

public void SpecifyTheCulture(CultureInfo culture, string propertyName)
{
propertyCultures[propertyName] = culture;
}

public void AddSerializationMethod<TType>(Func<TType, string> serializationMethod)
{
typeSerializationMethods[typeof(TType)] = obj => serializationMethod((TType)obj);
}

public void AddSerializationMethod<TType>(Func<TType, string> serializationMethod, string propertyName)
{
propertySerializationMethods[propertyName] = obj => serializationMethod((TType)obj);
}

public void AddLengthProperty(string propertyName, int trimLength)
{
propertyLengths[propertyName] = trimLength;
}

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

if (!finalTypes.Contains(type) && !type.IsPrimitive)
{
serializedFinalType = null;
return false;
}

serializedFinalType = obj switch
{
IFormattable format when typeCultures.TryGetValue(type, out var formatProvider) =>
format.ToString(null, formatProvider),
IFormattable format =>
format.ToString(null, CultureInfo.InvariantCulture),
string str => str,
_ => Convert.ToString(obj)
};

serializedFinalType += Environment.NewLine;

return true;
}

private bool TrySerializeCollection(object obj, int nestingLevel, out string? serializedCollection)
{
if (obj is not IEnumerable collection)
{
serializedCollection = null;
return false;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var type = obj.GetType();

collectionOnString.AppendLine($"{type.Name}:");

foreach (var item in collection)
{
collectionOnString.Append(indentation + PrintToString(item, nestingLevel + 1));
}

serializedCollection = collectionOnString.ToString();
return true;
}

private string SerializeComplexType(object obj, int nestingLevel)
{
processedObjects.Add(obj);
var serializedString = new StringBuilder();
var indentation = new string('\t', nestingLevel + 1);
var type = obj.GetType();

serializedString.AppendLine(type.Name);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 ?
Если не будет, корректное ли это поведение?
Если не будет, есть ли какие-то другие места с похожими потенциальными проблемами?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
sb.Append(identation + propertyInfo.Name + " = " +
PrintToString(propertyInfo.GetValue(obj),
nestingLevel + 1));
continue;
}
return sb.ToString();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return serializedString.ToString();
}

private string SerializeProperty(PropertyInfo property, object obj, int nestingLevel)
{
var propertyValue = property.GetValue(obj);

if (typeSerializationMethods.TryGetValue(property.PropertyType, out var serializationType))
{
return serializationType(propertyValue!) + Environment.NewLine;
}

if (propertySerializationMethods.TryGetValue(property.Name, out var serializationProperty))
{
return serializationProperty(propertyValue!) + Environment.NewLine;
}

if (propertyLengths.TryGetValue(property.Name, out var length))
{
var stringPropertyValue = Convert.ToString(propertyValue)!;

return stringPropertyValue[..length] + Environment.NewLine;
}

if (propertyCultures.TryGetValue(property.Name, out var formatProvider) && propertyValue is IFormattable format)
{
return format.ToString(null, formatProvider) + Environment.NewLine;
}

return PrintToString(propertyValue, nestingLevel);
}

private static string GetPropertyName<T>(Expression<Func<TOwner, T>> property)
{
if (property.Body is MemberExpression member)
{
return member.Member.Name;
}

throw new ArgumentException("Expression is not property.");
}
}
46 changes: 46 additions & 0 deletions ObjectPrinting/PropertyPrintingConfig.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
using System;
using System.Globalization;

namespace ObjectPrinting;

public class PropertyPrintingConfig<TOwner, TPropType>
{
private readonly PrintingConfig<TOwner> printingConfig;
protected internal readonly string? PropertyName;

public PropertyPrintingConfig(PrintingConfig<TOwner> printingConfig, string? propertyName = null)
{
this.printingConfig = printingConfig;
PropertyName = propertyName;
}

public PrintingConfig<TOwner> Using(Func<TPropType, string> print)
{
if (string.IsNullOrEmpty(PropertyName))
{
printingConfig.AddSerializationMethod(print);
}
else
{
printingConfig.AddSerializationMethod(print, PropertyName);
}

return ParentConfig;
}

public PrintingConfig<TOwner> Using(CultureInfo culture)
{
if (string.IsNullOrEmpty(PropertyName))
{
printingConfig.SpecifyTheCulture<TPropType>(culture);
}
else
{
printingConfig.SpecifyTheCulture(culture, PropertyName);
}

return ParentConfig;
}

public PrintingConfig<TOwner> ParentConfig => printingConfig;
}
10 changes: 0 additions & 10 deletions ObjectPrinting/Solved/ObjectExtensions.cs

This file was deleted.

10 changes: 0 additions & 10 deletions ObjectPrinting/Solved/ObjectPrinter.cs

This file was deleted.

Loading