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

Бабинцев Григорий #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qreaqtor
Copy link

Бабинцев Григорий - дз object printer

Copy link

@Buskervil Buskervil left a comment

Choose a reason for hiding this comment

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

  1. Можно сделать аккуратнее (иморты, орфография, nullable types)
  2. Нужно добавить обработку полей и циклических ссылок

.PrintToString(person);

result.Should().NotContain(nameof(person.Id));
result.Should().NotContain($"{person.Id}");

Choose a reason for hiding this comment

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

Этот тест можно сделать более строгим. если проверить, что id друга тоже нет в строке

Choose a reason for hiding this comment

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

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


person = new Person
{
Id = new Guid(),

Choose a reason for hiding this comment

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

В контексте этих тестов это не важно, но немного странно, что ты используешь пустой guid. Обычно берут Guid.NewGuid();

using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text;

Choose a reason for hiding this comment

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

чутка лишних импортов. Это не страшно, но добавляет неаккуратности в код

Choose a reason for hiding this comment

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

в других файлах тоже удали

[TestCase("Donatello", 4, "Dona")]
public void PrintToString_WithTrimLengthForString(string name, int maxLength, string expected)
{
person = new Person

Choose a reason for hiding this comment

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

Повесь на класс атрибут [Parallelizable(ParallelScope.All)] и запусти все тесты раз 10, увидишь, что они случайным образом падают)

У тебя person один на все тесты. Лучше не менять его вот так, чтобы не получить странные эффекты между вызовами тестов. Здесь лучше использовать локальную переменную (старайся делать так, чтобы тесты не аффектили друг друга при параллельном запуске)

result1.Should().Contain("Ilya");

result2.Should().NotContain("Ilya");
result2.Should().Contain("Home");

Choose a reason for hiding this comment

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

Интересно). Я не уверен, что это интуитивно понятное поведение., еще если настройка очень сложная и часто выполняется то будет много мусора создаваться (ссылочных типов).
Но молодец, что тестом задокументировал)

using System.Text;
using System.Threading.Tasks;

namespace ObjectPrinting.Tests

Choose a reason for hiding this comment

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

Будет максимально круто, если зафиксируешь, что настройка сериализации для члена класса имеет приоритет над настройкой для типа

foreach (DictionaryEntry keyValuePair in dictionary)
{
identation = new string('\t', nestingLevel + 1);
sb.Append(identation + "[" + PrintToString(keyValuePair.Key, nestingLevel + 1).Trim() + " - ");

Choose a reason for hiding this comment

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

Можно использовать интерполяцию или sb.
StringBuilder кстати тоже имеет fluentAPI и метод AppendLine там есть)

var sb = new StringBuilder();
var identation = new string('\t', nestingLevel);
sb.Append(identation + "{" + Environment.NewLine);
foreach (DictionaryEntry keyValuePair in dictionary)

Choose a reason for hiding this comment

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

Честно говоря, не понятно как оно работает. А ты понимаешь? Мб надежнее обходить ключи?

return serrialize.DynamicInvoke(obj) + Environment.NewLine;

if (obj is ICollection collection)
return SerializeCollection(collection, nestingLevel);

Choose a reason for hiding this comment

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

Вообще на это можно посмотреть как на предопределенный сериализатор по типу. И было бы здорово уметь определять такие сбоку от этого класса, чтобы не приходилось модифицировать его при добавлении новых сериализаторов. Но это так, размышления)... делать не надо

@@ -5,6 +5,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="FluentAssertions" Version="7.0.0" />

Choose a reason for hiding this comment

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

Для тестов лучше иметь отдельный проект, чтобы не тащить лишние зависимости в основную сборку

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