Conversation
GlazProject
left a comment
GlazProject
left a comment
There was a problem hiding this comment.
За исключением нескольких комментариев получилось неплохо. Код читаемый и переиспользуемый
There was a problem hiding this comment.
Хорошо что вынес все тесты в отдельный проект. Это позволит поддерживать чистым проект с бизнес логикой, добавляя там свои точки входа. А всё тестирование в отдельном месте. Плюс, можно проверять, как себя поведёт библиотека на разных версиях .net
| public class Person | ||
| { | ||
| public Guid Id { get; set; } | ||
| public string Name { get; set; } |
There was a problem hiding this comment.
Можно сделать полем, чтобы проверить обработку не только свойств
ObjectPrinting.Tests/Tests.cs
Outdated
| namespace ObjectPrinting.Tests | ||
| { | ||
| [TestFixture] | ||
| public class Tests |
There was a problem hiding this comment.
Всё-таки класс стоит называть по имени тестируемого объекта. В данном случае это был бы ObjectPrinterTests
ObjectPrinting.Tests/Tests.cs
Outdated
| person = new Person | ||
| { | ||
| Id = new Guid("3f2504e0-4f89-11d3-9a0c-0305e82c3301"), | ||
| Age = 39, | ||
| Name = "Dima", | ||
| Height = 176.0, | ||
| BirthDate = new DateTime(1985, 07, 27) | ||
| }; |
There was a problem hiding this comment.
Создание человека можно вынести в OneTimeSetUp и в тестах не изменять его значений. Лучше сразу же проинициализировать все массивы, чтобы не возникало необходимости в конкретных тестах
ObjectPrinting.Tests/Tests.cs
Outdated
| DefaultSettings.UseDirectory("ForVerifier"); | ||
| person = new Person | ||
| { | ||
| Id = new Guid("3f2504e0-4f89-11d3-9a0c-0305e82c3301"), |
There was a problem hiding this comment.
При использовании verify остаётся только такой способ создания гуидов, это правда. Но в целом гуиды используются как условно уникальные идентификаторы, поэтому их создание выглядит как Guid.NewGuid(). Тут ничего менять не нужно
| return printingConfig; | ||
| } | ||
|
|
||
| //public PrintingConfig<TOwner> Using(CultureInfo culture) |
| PrintingConfig<TOwner> IPropertyPrintingConfig<TOwner, TPropType>.ParentConfig => printingConfig; | ||
| } | ||
|
|
||
| public interface IPropertyPrintingConfig<TOwner, TPropType> |
There was a problem hiding this comment.
Здесь можно было не писать интерфейс, так как другой реализации для конфига не предвидится, а в интерфейсе фиксируется не только логика, но и внутреннее поведение (явная декларация внутреннего конфига)
| public static string PrintToString<T>(this T obj, Func<PrintingConfig<T>, PrintingConfig<T>> config) | ||
| { | ||
| return config(ObjectPrinter.For<T>()).PrintToString(obj); | ||
| } |
| CultureInfo culture) | ||
| where TPropType : IFormattable | ||
| { | ||
| return config.Using(x => x.ToString(null, culture)); |
There was a problem hiding this comment.
Очень хорошо, что увидел возможность использования конфигов здесь
| public static PrintingConfig<TOwner> TrimmedToLength<TOwner>(this PropertyPrintingConfig<TOwner, | ||
| string> propConfig, int maxLen) | ||
| { | ||
| return propConfig.Using(x => x[..Math.Min(x.Length, maxLen)]); |
There was a problem hiding this comment.
А вот эта строчка опасна. Такой срез будет генерировать каждый раз новую строку. Давай лучше проверять через if, и только при необходимости возвращать обрезанную
| if (obj is not ICollection) return PrintObj(obj, nestingLevel); | ||
| var collection = (IEnumerable) obj; |
There was a problem hiding this comment.
Проверяем сначала на ICollection, а потом приводим к IEnumerable. Такое возможно со стороны типов, но логически выглядит конструкция странно. Рекомендуется использовать одновременный каст и генерацию новой переменной нужного типа вот таким образом
if (obj is not ICollection collection)
return PrintObj(obj, nestingLevel);
return PrintCollection(collection, nestingLevel);
Или даже через тернарный оператор
return obj is ICollection collection
? PrintCollection(collection, nestingLevel)
: PrintObj(obj, nestingLevel);
| foreach (var memberInfo in type | ||
| .GetProperties() | ||
| .Cast<MemberInfo>() | ||
| .Concat(type.GetFields(BindingFlags.Instance | BindingFlags.Public))) |
There was a problem hiding this comment.
Сложная конструкция источника данных, лучше вынести в отдельную переменную
| return new PropertyPrintingConfig<TOwner, TPropType>(this); | ||
| } | ||
|
|
||
| public PropertyPrintingConfig<TOwner, TPropType> Printing<TPropType>(Expression<Func<TOwner, TPropType>> memberSelector) |
2 participants
@glazz200