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

Овечкин Илья #238

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

Conversation

magnat0
Copy link

@magnat0 magnat0 commented Nov 28, 2023

@DonielPankek

Copy link

@MrTimeChip MrTimeChip left a comment

Choose a reason for hiding this comment

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

Достаточно странные проблемы с форматированием, табуляцией. Можно такое поправить даже авторефакторингом. В остальном - очень даже неплохо, как поправишь, посмотрю снова.

[TestCase(3, 2, true, "a.sd", false)]
[TestCase(3, 2, true, "", false)]
[TestCase(3, 2, true, null, false)]
public void ValidateNumberCorrect_When_DaraCorrect(int precision, int scale, bool onlyPositive, string value, bool expectedResult)

Choose a reason for hiding this comment

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

Data, а не Dara.

Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, false));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
[Test]

Choose a reason for hiding this comment

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

Поехало форматирование - нет табуляции после "{" у класса.

Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, false));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
[Test]

Choose a reason for hiding this comment

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

При использовании [TestCase] атрибут [Test] не нужен, вроде как.

[TestCase(1, -2, false)]
[TestCase(1, 2, true)]
[TestCase(1, 1, false)]
public void Should_ThrowArgumentException_When_InitIncorrectData(int precision, int scale, bool onlyPositive)

Choose a reason for hiding this comment

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

Не совсем понятно "InitIncorrectData", можно без init.

public void CheckCurrentTsar()
{
var actualTsar = TsarRegistry.GetCurrentTsar();
[Test]

Choose a reason for hiding this comment

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

Тут тоже слетело форматирование - не хватает табуляции.

Comment on lines 11 to 22
[TestCase(17, 2, true, "0.0", true)]
[TestCase(17, 2, true, "0", true)]
[TestCase(3, 2, true, "00.00", false)]
[TestCase(3, 2, true, "-0.0", false)]
[TestCase(3, 2, true, "+0.00", false)]
[TestCase(4, 2, true, "+1.23", true)]
[TestCase(3, 2, true, "+1.23", false)]
[TestCase(17, 2, true, "0.000", false)]
[TestCase(3, 2, true, "-1.23", false)]
[TestCase(3, 2, true, "a.sd", false)]
[TestCase(3, 2, true, "", false)]
[TestCase(3, 2, true, null, false)]

Choose a reason for hiding this comment

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

Все кейсы тут не учитывают один из параметров, он всегда одинакового значения. Давай напишем ещё кейсов, где этот параметр тоже будет изменен?

Comment on lines 11 to 22
[TestCase(17, 2, true, "0.0", true)]
[TestCase(17, 2, true, "0", true)]
[TestCase(3, 2, true, "00.00", false)]
[TestCase(3, 2, true, "-0.0", false)]
[TestCase(3, 2, true, "+0.00", false)]
[TestCase(4, 2, true, "+1.23", true)]
[TestCase(3, 2, true, "+1.23", false)]
[TestCase(17, 2, true, "0.000", false)]
[TestCase(3, 2, true, "-1.23", false)]
[TestCase(3, 2, true, "a.sd", false)]
[TestCase(3, 2, true, "", false)]
[TestCase(3, 2, true, null, false)]

Choose a reason for hiding this comment

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

Давай ещё всем кейсам добавим понятные названия, чтобы можно было понять, что именно конкретный кейс означает с точки зрения логики?

Assert.AreEqual(actualTsar.Height, expectedTsar.Height);
Assert.AreEqual(actualTsar.Weight, expectedTsar.Weight);
actualTsar.Should().BeEquivalentTo(expectedTsar, options =>
options.Excluding(option => option.Id)

Choose a reason for hiding this comment

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

Тут option не совсем корректно, как мне кажется. Здесь мы поля выбираем у объекта, все таки, а не у options.

@MrTimeChip
Copy link

Все хорошо, 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