-
Notifications
You must be signed in to change notification settings - Fork 245
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
Вильданов Савелий #218
base: master
Are you sure you want to change the base?
Вильданов Савелий #218
Conversation
ObjectPrinting/PrintingConfig.cs
Outdated
alternativeSerializeProperties[propertyInfo] = o => serializeFunc((T) o); | ||
} | ||
|
||
public void AddLengthOfProperty(PropertyInfo propertyInfo, int length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот метод не должен быть доступен для потребителя
ObjectPrinter.For<Person>().AddLengthOfProperty(); //не должно быть доступа
ObjectPrinting/PrintingConfig.cs
Outdated
return propertyInfo; | ||
} | ||
|
||
public void AddSerializedProperty<T>(PropertyInfo propertyInfo, Func<T, string> serializeFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот метод не должен быть доступен для потребителя
ObjectPrinter.For<Person>().AddSerializedProperty(); //не должно быть доступа
ObjectPrinting/PrintingConfig.cs
Outdated
return this; | ||
} | ||
|
||
public PrintingConfig<TOwner> SerializeTypeWithSpecial<T>(Func<object, string> serializeFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- По заданию подразумевалось что вызов этого метода будет после выбора типа или свойства:
ObjectPrinter.For<Person>()
.For<int>().Serialize(...)
...
ObjectPrinter.For<Person>()
.For(x => x.Age).Serialize(...)
...
- У нас вроде generic-метод, а почему в принимаем функцию от object?
ObjectPrinting/PrintingConfig.cs
Outdated
return this; | ||
} | ||
|
||
public PrintingConfig<TOwner> SelectCulture<T>(CultureInfo cultureInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- По заданию: "Для всех типов, имеющих культуру, есть возможность ее указать".
- По примеру из кода подразумевался такой вызов:
ObjectPrinter.For<Person>()
.For<double>().UseCulture(...)
...
public class PropertyPrintingConfig<TOwner, TPropType> | ||
{ | ||
public readonly PrintingConfig<TOwner> PrintingConfig; | ||
public readonly PropertyInfo PropertyInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
К этим полям не должно быть доступа для потребителя
ObjectPrinter.For<Person>().SelectField(x => x.Id).PrintingConfig; //не должно быть доступа
ObjectPrinter.For<Person>().SelectField(x => x.Id).PropertyInfo; //не должно быть доступа
ObjectPrinting/PrintingConfig.cs
Outdated
@@ -14,28 +33,160 @@ public string PrintToString(TOwner obj) | |||
private string PrintToString(object obj, int nestingLevel) | |||
{ | |||
//TODO apply configurations | |||
var identation = new string('\t', nestingLevel + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Очепятка: indentation
ObjectPrinting/PrintingConfig.cs
Outdated
} | ||
|
||
|
||
private string CheckSerializationConditions(object obj, string identation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно пометить, что object может быть nullable символом ?
:
private string CheckSerializationConditions(object? obj, string identation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что-то не так с методом:
- Параметр identation не используется.
- Называется метод "Проверь условия сериализации", но при этом он возвращает сериализованную строку, а в каких-то случаях
null
.
ObjectPrinting/PrintingConfig.cs
Outdated
if (obj == null) | ||
return "null" + Environment.NewLine; | ||
if (excludedTypes.Contains(obj.GetType())) | ||
return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return string.Empty;
@@ -14,28 +33,160 @@ public string PrintToString(TOwner obj) | |||
private string PrintToString(object obj, int nestingLevel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что-то не так с алгоритмом: у нас есть метод CheckSerializationConditions
и логика в нём частично повторяется тут. Мб как-то можно более проще сделать?
ObjectPrinting/PrintingConfig.cs
Outdated
sb.Append(identation + propertyInfo.Name + " = " + | ||
propertyFunc(propertyInfo.GetValue(obj)) + Environment.NewLine); | ||
else | ||
sb.Append(identation + propertyInfo.Name + " = " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если у тебя много переносов, значит что-то не чисто:
- Не экономь на переменных - это бесплатно:
var serializedObj = ...
sb.Append(serializedObj);
- Не гонись за "действиями в одну строку" - если потребуется в условную конструкцию добавить скобки - ничего страшного.
|
||
namespace ObjectPrinting.Tests | ||
{ | ||
public class ObjectPrinterTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По-хорошему тесты в отдельный проект выносят - но на это могу в целом закрыть глаза.
В следующих домашках уже буду требовать.
private Person person; | ||
|
||
[SetUp] | ||
public void Setup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не вижу смысла в этом SetUp: у тебя статический объект, который не меняется - его можно вынести в поля класса:
private static readonly Person Person = new()
{ Name = "Monkey", SecondName = "D.Luffy", NameOfPet = "Usopp", Height = 1234567.89, Age = 17 };
} | ||
|
||
[Test] | ||
public void ObjetctPrinter_ShouldCorrectPrint_WithOutExcludingType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему ...Without...
?
Тут же как раз с ним - логичнее ObjetctPrinter_ShouldCorrectPrint_WithExcludingType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поправь и в других тестах.
result.Should().NotContain(person.Name) | ||
.And.NotContain(person.SecondName) | ||
.And.NotContain(person.NameOfPet); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Проблемы всех тестов:
- Не соблюдается паттерн ААА, все тесты должны выглядеть так:
{
//блок подготовки данных
var person = ...
var printer = ....
var expected = ...
//блок действия
var actual = ...
//блок проверки данных
actual.Should(). ...
}
- Используй var - пускай машина думает.
var printer = ObjectPrinter.For<Person>() | ||
.SelectCulture<double>(new CultureInfo("fr-FR")); | ||
string result = printer.PrintToString(person); | ||
result.Should().Contain("1\u00a0234\u00a0567,890"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Давай тут попроще какие-нить кейсы)
- Проверка, что при одной культуре у нас double сериализуется с точкой "1.1".
- Проверка, что при другой культуре у нас double сериализуется с запятой "1,1".
var printerRu = ObjectPrinter.For<Person>() | ||
.Printing<double>().Using(new CultureInfo("ru-Ru")); | ||
|
||
printerEn.PrintToString(Person).Should().Contain("168.8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Раздели это на 2 тест-кейса через атрибут [TestCase]
{Name = "Monkey", SecondName = "D.Luffy", NameOfPet = "Usopp", Height = 168.8, Age = 17}; | ||
|
||
[Test] | ||
public void ObjetctPrinter_ShouldCorrectPrint_WithExcludingType() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- В наименовании всех тестов опечатка
Objetct
- Во всех тестах блок проверки перемешался с блоком действия.
} | ||
|
||
[Test] | ||
public void ObjetctPrinter_ShouldCorrectPrint_WithDictionary() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Во всех тестах на коллекцию разделить на блоки AAA
@SlavikGh0st