Skip to content

Conversation

@HardreaM
Copy link

{
[Test]
[Description("Проверка текущего царя")]
[Category("ToRefactor")]

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.

А Description зачем удалил? )

@dmnovikova
Copy link

Разбиение тест кейсов по тестам - хорошее, но их сейчас мало. Например, сразу бросается в глаза, что тестов с использованием "+/-" очень мало; например, влияет ли знак на "total length > precision"?

Предлагаю увеличить количество проверок до 40-50

@dmnovikova
Copy link

Немного пожеланий:

  • перед отправкой на ревью форматируй код, приводя его к общепринятому стилю (для форматирования удобно использовать хоткеи решарпера: ctrl+alt+l или ctrl+alt+shift+l), это позволит снизить количество правок на ревью - например, уберет лишние юзинги и пробелы :)

  • так же обращай внимание на подсказки решарпера и не бойся рефачить смежный код. например, в Person половина свойств "светятся" подсказками

[TestCase("-1.13", 4, 2, false, TestName = "negative and onlyPositive = false")]
[TestCase("+1.13", 4, 2, true, TestName = "has plus and onlyPositive = true")]
[TestCase("+1.13", 4, 2, false, TestName = "has plus and onlyPositive = false")]
public void IsValidNumber_CheckValidNumber_ReturnsTrue(string value, int precision, int scale = 2,

Choose a reason for hiding this comment

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

Тут и в соседних тестах получилось слишком много тесткейсов, из-за этого сложно отслеживать все ли случаи учтены и находить упавший — страдает читаемость кода
Большой подсказкой, что некоторые тесткейсы нужно разнести по разным тестам является схожее описание в TestName


public bool IsValidNumber(string value)
{
// Проверяем соответствие входного значения формату N(m,k), в соответствии с правилом,

Choose a reason for hiding this comment

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

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

@dmnovikova
Copy link

Небольшие замечания по работе с гитом:

  • Разбивать изменения на коммиты следует исходя из смысла этих изменений, а не пофайлово, чтобы каждый коммит представлял собой одно небольшое логически завершенное действие (как правило, такой код должен билдиться). Понимание нужного размера приходит с опытом, сейчас нужно постараться их не делать очень большими или очень маленькими.

  • Лишние изменения не должны попадать в коммит. Например, не нужно добавлять новый парсер, и при этом рефакторить какой-нибудь легаси в одном коммите. Так и с тестами :) - лучше форматирование и добавление новых случаев разбивать на отдельные коммиты.

  • Ну и название коммита должно отражать содержимое — тут все хорошо )

Зачем все это?
Потому что ревьюеру выделить определенную идею среди множества изменений непросто, время, потраченное на ревью увеличивается в несколько раз. Поэтому коммитить пофайлово нет смысла, тк это не решает проблему.
А еще в работе не так уж редко приходится обращаться к истории коммитов, чтобы разобраться в малознакомом коде.

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