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

Рушкова Ольга #230

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SomEnaMeforme
Copy link

No description provided.

Comment on lines 20 to 22
<ItemGroup>
<Folder Include="Tests\TestCases\" />
</ItemGroup>

Choose a reason for hiding this comment

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

Лишнее

using System.Text;

namespace ObjectPrinting
{
public class ArrayPrintingConfig<T> : PrintingConfig<T[]>

Choose a reason for hiding this comment

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

Давай вынесем в отдельный файл. Лучше придерживаться принципа 1 класс - 1 файл

Choose a reason for hiding this comment

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

Или же можно вовсе избавиться от этого класса, вместо можно сделать метод PrintToStringCollection публичным

Comment on lines 26 to 29
protected HashSet<Type> excludedTypes = [];
protected HashSet<string> excludedProperties = [];
protected Dictionary<string, Func<object, string>> propertySerializers = [];
protected Dictionary<Type, Func<object, string>> propertyTypeSerializers = [];

Choose a reason for hiding this comment

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

protected поля/свойства с большой буквы по код стайлу

protected HashSet<string> excludedProperties = [];
protected Dictionary<string, Func<object, string>> propertySerializers = [];
protected Dictionary<Type, Func<object, string>> propertyTypeSerializers = [];
private HashSet<Type> finalTypes = new HashSet<Type>()

Choose a reason for hiding this comment

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

Можно сделать readonly, так как меняться эта коллекция никогда не должна

Comment on lines 17 to 21
protected PropertyPrintingConfig<TParent, TProperty> RefineSerializer(Func<string, string> serializer)
{
parentConfig.RefineSerializer<TProperty>(serializer);
return this;
}

Choose a reason for hiding this comment

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

Не используется

return sb.ToString().Trim();
}

private string PropertyToString(

Choose a reason for hiding this comment

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

Можно попробовать переделать на метод принимающий MemberInfo. PropertyInfo и FiledInfo можно скастить к MemberInfo и одинаково с ними работать. А логику в различии поля и свойства вынести в отдельное место, например как extensions

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

public PropertyPrintingConfig<TOwner, TProperty> Printing<TProperty>(Func<TOwner, TProperty> get)

Choose a reason for hiding this comment

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

Название сбивает с толку - printer.Printing(...) хочется сразу думать что сейчас уже будет вывод строки, но нет

[UseReporter(typeof(DiffReporter))]
public class ObjectPrinterTests
{
[Test] //1

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.

Чтобы было проще отслеживать кейсы из задания - не потерялся ли функционал, удалила

@ksamnole
Copy link

ksamnole commented Dec 14, 2024

В общем, всё что было в задании выполнено, но попробуй немного порефакторить своё решение. Важна ведь не только правильная работа кода, но и его "чистота". Если смотреть с точки зрения разработки в больших проектах, то твой код будет жить "вечно", его должны переиспользовать люди и быстро разбираться, что происходит. Но так-то всё супер!

Пока один балл поставлю, потом исправлю на 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants