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.

Sign up for GitHub

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

Трофимов Никита #239

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

Трофимов Никита #239

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fda963
Отрефакторил NumberValidator и ObjectComparsion, добавил новые тесты …
WoeromProg Nov 28, 2023
b1c745d
Изменил название тестов
WoeromProg Nov 30, 2023
6a3a718
Изменил имена файлов
WoeromProg Nov 30, 2023
6cbad54
Изменил имена файлов без опечатки (ObjectComparison)
WoeromProg Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 45 additions & 26 deletions cs/HomeExercises/NumberValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,59 @@
using System;
using System.Diagnostics.SymbolStore;
using System.Text.RegularExpressions;
using FluentAssertions;
using NUnit.Framework;
using NUnit.Framework.Internal;

namespace HomeExercises
{
public class NumberValidatorTests
Copy link

Choose a reason for hiding this comment

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

Во всех методах этого класса потерялась часть, означающая "при каком условии выполняется действие", то есть часть, начинающаяся на _When.... В случае с TestCase желательно обобщить все тесткейсы под одно выражение и записать его в название метода, а сами тесткейсы уже будут пояснять детали своими названиями

{
[Test]
Copy link

Choose a reason for hiding this comment

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

Если есть TestCase, то [Test] писать не нужно, это почти синонимы. Тут и во всех тестах ниже

public void Test()
[TestCase(-1, 2, true, TestName = "Creation_ShouldThrowArgumentException_WhenNegativePrecision")]
Copy link

Choose a reason for hiding this comment

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

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

  1. Базовая часть названия уже есть в названии метода, дублировать её просто не имеет смысла.
    image

Почему так? Читая тест кейсы в инспекторе, мы и так понимаем, что находимся внутри метода Creation_ShouldThrowArgumentException, поэтому дублирование только создает лишнюю когнитивную нагрузку. Также при переименовании метода, TestName автоматом не обновится, если это не сделает IDE

  1. Когда удалится дублирование, останутся фразы вида When..., от них на самом деле тоже можно избавиться, оставив только самую суть - что проверяет данный тесткейс

  2. Поправить нужно тут и во всех тестах ниже

[TestCase(0,0,true, TestName = "Creation_ShouldThrowArgumentException_WhenPrecisionIsZero")]
Copy link

Choose a reason for hiding this comment

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

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

[TestCase(3,-1,true, TestName = "Creation_ShouldThrowArgumentException_WhenNegativeScale")]
[TestCase(1,2,true, TestName = "Creation_ShouldThrowArgumentException_WhenScaleIsGreaterThanPrecision")]
Copy link

Choose a reason for hiding this comment

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

Не хватает ещё одного граничного, очень похожего на этот ))

public void Creation_ShouldThrowArgumentException(int precision, int scale, bool onlyPositive)
Copy link

Choose a reason for hiding this comment

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

Для чего в тесте аргумент onlyPositive, если он всегда равен true?

{
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, true));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, false));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
Action creation = () => new NumberValidator(precision, scale, onlyPositive);
creation.Should().Throw<ArgumentException>();
}

[Test]
[TestCase(1, 0, true, TestName = "Creation_ShouldDoesNotThrow_WhenCorrectValue")]
Copy link

Choose a reason for hiding this comment

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

Если TestCase всего один, то это просто Test )

public void Creation_ShouldDoesNotThrow(int precision, int scale, bool onlyPositive)
Copy link

Choose a reason for hiding this comment

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

Я вообще ни капли не англичанин в последней инстанции, но не встречал употребления does после should, для понимания хватает просто ShouldNotThrow

{
Action creation = () => new NumberValidator(precision, scale, onlyPositive);
creation.Should().NotThrow<ArgumentException>();
Copy link

Choose a reason for hiding this comment

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

А для чего завязался именно на эту ошибку? Если выбросится какой-нибудь NRE, то тест отработает корректно, но этого ли мы добиваемся тут?

}

Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("00.00"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-0.00"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("+0.00"));
Assert.IsTrue(new NumberValidator(4, 2, true).IsValidNumber("+1.23"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("+1.23"));
Assert.IsFalse(new NumberValidator(17, 2, true).IsValidNumber("0.000"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd"));
[Test]
[TestCase(3,2,true, "-+0.0", TestName = "IsValidNumber_ReturnsFalseWhenTwoSigns")]
Copy link

Choose a reason for hiding this comment

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

Не хватает ещё нескольких тестов на логику работы, на самом деле они вытекают из параметров конструктора, давай допишем

[TestCase(3,2,true, " -0.0", TestName = "IsValidNumber_ReturnsFalseWhenSpaceFirstDigitInNumber")]
[TestCase(3,2,true, "0,", TestName ="IsValidNumber_ReturnsFalseWhenAren'tDigitsAfterCommas")]
[TestCase(3,2,true, "0.0.0", TestName = "IsValidNumber_ReturnsFalseWhenThreeDigitsInNumberByTwoDots")]
[TestCase(3,2,true, "0,0,0", TestName = "IsValidNumber_ReturnsFalseWhenThreeDigitsInNumberByTwoCommas")]
[TestCase(3, 2, true, "00.00", TestName = "IsValidNumber_ReturnsFalseWhenNumberPrecisionGreaterThanValidatorPrecision")]
[TestCase(3, 2, true, null, TestName = "IsValidNumber_ReturnsFalseWhenNumberNull")]
[TestCase(3, 2, true, "", TestName = "IsValidNumber_ReturnsFalseWhenNumberEmpty")]
[TestCase(3, 2, true, "+1.23", TestName = "IsValidNumber_ReturnsFalseWhenNumberPrecisionGreaterThanValidatorPrecision")]
[TestCase(3, 2, true, "-1.23", TestName = "IsValidNumber_ReturnsFalseWhenNumberNegativeInNumberOnlyPositive")]
[TestCase(17, 2, true, "0.000", TestName = "IsValidNumber_ReturnsFalseWhenNumberScaleGreaterThanValidatorScale")]
[TestCase(17, 2, true, "a.sd", TestName = "IsValidNumber_ReturnsFalseWhenIncorrectNumberFormatBecauseLettersArePresent")]
public void IsValidNumber_False(int precision, int scale, bool onlyPositive, string number)
Copy link

Choose a reason for hiding this comment

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

В названии теста потерялся глагол перед False

{
NumberValidator validator = new NumberValidator(precision, scale, onlyPositive);
validator.IsValidNumber(number).Should().BeFalse();
}
[Test]
[TestCase(17, 2, true, "0.0", TestName = "IsValidNumber_ReturnTrueWhenNumberWithFractionalPart")]
Copy link

Choose a reason for hiding this comment

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

Точно ли здесь учтены все случаи корректной работы метода?

[TestCase(17, 2, true, "0", TestName = "IsValidNumber_ReturnTrueWhenOnlyInteger")]
[TestCase(4, 2, true, "+1.23", TestName = "IsValidNumber_ReturnTrueWhenNumberWithSign")]
public void IsValidNumber_True(int precision, int scale, bool onlyPositive, string number)
Copy link

Choose a reason for hiding this comment

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

Аналогичный комментарий с неймингом, как для теста выше

{
NumberValidator validator = new NumberValidator(precision, scale, onlyPositive);
validator.IsValidNumber(number).Should().BeTrue();
}
}

Copy link

@desolver desolver Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Понимаю, что этот файл создавал не ты, но быть может задумка автора как раз и была в том, чтобы с какими-то конвентами знакомить

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

Аналогично с соседним файлом

Expand All @@ -51,22 +78,14 @@ public NumberValidator(int precision, int scale = 0, bool onlyPositive = false)

public bool IsValidNumber(string value)
{
// Проверяем соответствие входного значения формату N(m,k), в соответствии с правилом,
// описанным в Формате описи документов, направляемых в налоговый орган в электронном виде по телекоммуникационным каналам связи:
// Формат числового значения указывается в виде N(m.к), где m – максимальное количество знаков в числе, включая знак (для отрицательного числа),
// целую и дробную часть числа без разделяющей десятичной точки, k – максимальное число знаков дробной части числа.
// Если число знаков дробной части числа равно 0 (т.е. число целое), то формат числового значения имеет вид N(m).

if (string.IsNullOrEmpty(value))
return false;

var match = numberRegex.Match(value);
if (!match.Success)
return false;

// Знак и целая часть

var intPart = match.Groups[1].Value.Length + match.Groups[2].Value.Length;
// Дробная часть
var fracPart = match.Groups[4].Value.Length;

if (intPart + fracPart > precision || fracPart > scale)
Expand Down
18 changes: 7 additions & 11 deletions cs/HomeExercises/ObjectComparison.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,9 @@ public void CheckCurrentTsar()
var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70,
Copy link

Choose a reason for hiding this comment

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

Не могу почему-то написать комментарий парой строк выше)) Напишу тут - понятное ли название у теста?

new Person("Vasili III of Russia", 28, 170, 60, null));
Copy link

Choose a reason for hiding this comment

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

Замени здесь Василия 3, на Василия 2 и прогони тест, ожидаемое ли это поведение?


// Перепишите код на использование Fluent Assertions.
Assert.AreEqual(actualTsar.Name, expectedTsar.Name);
Assert.AreEqual(actualTsar.Age, expectedTsar.Age);
Assert.AreEqual(actualTsar.Height, expectedTsar.Height);
Assert.AreEqual(actualTsar.Weight, expectedTsar.Weight);

Assert.AreEqual(expectedTsar.Parent!.Name, actualTsar.Parent!.Name);
Assert.AreEqual(expectedTsar.Parent.Age, actualTsar.Parent.Age);
Assert.AreEqual(expectedTsar.Parent.Height, actualTsar.Parent.Height);
Assert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent);
actualTsar.Should().BeEquivalentTo(expectedTsar, options => options
.Excluding(tsar => tsar.Parent)
.Excluding(tsar => tsar.Id));
}

[Test]
Expand All @@ -35,7 +28,10 @@ public void CheckCurrentTsar_WithCustomEquality()
var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));

// Какие недостатки у такого подхода?
//Какие недостатки у такого подхода?
//Нужно будет переписывать AreEqual в случае изменения полей Person
//Возможна ошибка при написании метода
//Если тест упадет, сообщение об ошибке будет малоинформативным
Assert.True(AreEqual(actualTsar, expectedTsar));
}

Expand Down