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

Зиновьева Милана #216

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

Conversation

crycrash
Copy link

No description provided.

Comment on lines 26 to 31
public PrintingConfig<TOwner> Excluding<TPropType>(Expression<Func<TOwner, TPropType>> memberSelector)
{
if (memberSelector.Body is MemberExpression memberExpression)
excludedProperties.Add(memberExpression.Member.Name);
return this;
}
Copy link

Choose a reason for hiding this comment

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

Тут следует воткнуть ошибку, чтобы разработчик сходу понял, что сделал неправильно. Да, если экспрешн сгенерирован автоматически и не протестирован, то на проде выстрелит, но иначе мы введём клиента в замешательство, тк выдадим неожидаемое поведение, не сказав

Comment on lines 57 to 58
if (typeSerializers.ContainsKey(type))
return typeSerializers[type](obj) + Environment.NewLine;
Copy link

Choose a reason for hiding this comment

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

Тут тоже можно использовать TryGetValue

        if (typeSerializers.TryGetValue(type, out var serializer))
            return serializer(obj) + Environment.NewLine;

Comment on lines 63 to 67
var finalTypes = new[]
{
typeof(int), typeof(double), typeof(float), typeof(string),
typeof(DateTime), typeof(TimeSpan)
};
Copy link

Choose a reason for hiding this comment

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

Такая выборка неполная. Почему: ты забыла bool, char, long, byte, кучу других численных типов, включая типы без знака. Сверху добавляем енумы, которые клиент вообще сам может создать и отдать нам

Есть свойство Type.IsSerializable

На массиве ниже среди всех типов это поле было true, кроме созданных мной класса, рекорда и структуры. Выглядит надёжным

var finalTypes = new[]
        {
            typeof(int), typeof(double), typeof(float), typeof(string),
            typeof(DateTime), typeof(TimeSpan), typeof(bool), typeof(decimal), typeof(char), typeof(Guid), typeof(byte),
            typeof(long), typeof(uint), typeof(MyEnum), typeof(MyClass), typeof(MyStruct), typeof(MyRecord)
        };
var serializableTypes = finalTypes
    .Where(type => type.IsSerializable)
    .ToArray();


if (excludedTypes.Contains(propertyType) || excludedProperties.Contains(propertyName))
continue;
sb.Append(identation + propertyName + " = " +
Copy link

Choose a reason for hiding this comment

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

Не очень нравится, что везде стоит + Environment.NewLine

Если вот тут заменить на AppendLIne, то будет такая же структура, если не учитывать, что в конце не будет переноса, но это по идее даже лучше, тк перенос клиент сам поставит, если захочет. Не захочет - не поставит

PrintToString(propertyValue,
nestingLevel + 1));
}
return sb.ToString().Trim();
Copy link

Choose a reason for hiding this comment

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

А, переноса в конце и так не будет. Ну тогда тем более стоит просто воткнуть AppendLine. Я не увидел причин так не делать

Copy link

Choose a reason for hiding this comment

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

Кстати этот Trim стоит убрать, тк если последним полем будет строка, которая кончается пробельным символом, то он тоже обрежется

processedObjects.Add(obj);

if (excludedTypes.Contains(type))
return string.Empty;

This comment was marked as resolved.

This comment was marked as resolved.

return parentConfig;
}

public PrintingConfig<TOwner> TrimmedToLength(int maxLength)
Copy link

Choose a reason for hiding this comment

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

Тут стоит добавить возможность обрезать начало

@@ -13,4 +13,6 @@ public class Person
public DateTime DateBirth {get; set; }
public Person[] Parents { get; set; }
public Person[] Friends { get; set; }
public Dictionary<string, int> LimbToNumbersFingers {get; set;}
public List<Person> Childs {get; set;}
Copy link

Choose a reason for hiding this comment

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

докопался: дети по-английски - children, а словарь конечностей к числу пальцев - LimbToFingersCount

@@ -113,4 +118,34 @@ public void SetStringPropertyLength(string propertyName, int startIndex, int max
{
stringPropertyLengths[propertyName] = new Tuple<int, int>(startIndex, maxLength);
}

private string ProcessCollections(object obj, int nestingLevel){
if (obj is IDictionary dictionary)
Copy link

Choose a reason for hiding this comment

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

Ещё докопался: тут будет круче воткнуть IReadonlyDictionary, тк у него больше реализаций, но это будет всё тот же словарь, и если клиенту сковородка на голову упадёт, и он решит реализовать свой словарь, то он стопроц будет реализовывать IReadonlyDictionary (если сковородкой не так сильно огрело)

@@ -31,7 +31,7 @@ public void TestAllExcludingTypes()
.Excluding<string>().Excluding<int>().Excluding<double>().Excluding<Guid>().Excluding<DateTime>().Excluding<Person[]>()
.PrintToString(person);

result.Should().Be(excepted);
result.Trim().Should().Be(excepted);
Copy link

Choose a reason for hiding this comment

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

напрягают эти тримы. Теперь понял зачем ты везде тыкала newLine. Ну ладно, пусть будет перенос у пустого объекта

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