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

Бочаров Александр #222

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

Conversation

Geratoptus
Copy link

@Geratoptus Geratoptus commented Dec 11, 2024

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

var printingResult = GetPrintingResult(obj, memberInfo, nestingLevel);

sb.Append(indentation + memberInfo.Name + " = " + printingResult);

Choose a reason for hiding this comment

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

Вместо конкатенации лучше использовать интерполяцию

if (!IsPropertyOrField(member))
throw new ArgumentException("Provided member must be Field or Property");

return member.MemberType == MemberTypes.Field

Choose a reason for hiding this comment

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

Есть некоторая проблема... Верхний if проверяет на то что member может быть только двух типов, но в реальности у нас может быть ситуация, что кто-то по невнимательности изменит метод проверки IsPropertyOrField, забыв поменять здесь логику, в этом случае тернарник станет выкидывать, (сейчас могу соврать) CastException-ы, что не очень наглядно.

По коду вижу, что с is ты знаком, поэтому я бы рекомендовала использовать switch с pattern matching-ом, который опишет и логику извлечения данных и строго ограничит типы из которых эти данные можно извлечь. В этом случае весь этот метод выглядел бы как-то так:

private static object? GetMemberValue(MemberInfo member, Object obj)  
{  
        return member switch
        {
            _ when member is FieldInfo field => field.GetValue(obj),
            _ when member is PropertyInfo property => property.GetValue(obj),
            _ => throw new ArgumentException("Provided member must be Field or Property")
        };
}

Ну или так:

private static object? GetMemberValue(MemberInfo member, Object obj)
{
        return member switch
        {
            FieldInfo field => field.GetValue(obj),
            PropertyInfo property => property.GetValue(obj),
            _ => throw new ArgumentException("Provided member must be Field or Property")
        };
}

: ((PropertyInfo)member).GetValue(obj);
}

private static Type GetMemberType(MemberInfo member)

Choose a reason for hiding this comment

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

Ну, думаю, здесь понятно что нужно сделать)

PrintingConfig<TOwner> IMemberPrintingConfig<TOwner>.ParentConfig => PrintingConfig;
}

public interface IMemberPrintingConfig<TOwner>

Choose a reason for hiding this comment

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

Не очень хорошая история объявлять интерфейсы в файле класса - наследника, даже если он один. Лучше всё-таки вынести в отдельный файл

return GetPrintedSequence(obj, nestingLevel);
}

private string GetPrintedSequence(IEnumerable obj, int nestingLevel)

Choose a reason for hiding this comment

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

IEnumerable заслуживает чего-то большего чем просто obj) Ну а если серьёзно, то именование всё-таки важно, нижний цикл выглядит очень странно, будто бы мы пытаемся итерироваться по объекту...

@masssha1308
Copy link

26 коммитов... Не то чтобы я сильно против была, но у нас в проекте такое не любят. Иногда всё-таки лучше использовать amend

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