Козлов Данил #228
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,80 +1,119 @@ | ||
| using System; | ||
| using System.Text.RegularExpressions; | ||
| using FluentAssertions; | ||
| using FluentAssertions.Execution; | ||
| using NUnit.Framework; | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace HomeExercises | ||
| { | ||
| [TestFixture] | ||
| public class NumberValidatorTests | ||
| { | ||
| private static IEnumerable<TestCaseData> IsValidNumberPrecisionTests = new TestCaseData[] | ||
| { | ||
| new TestCaseData(3, 2, true, "+0.00").Returns(false) | ||
| .SetName("False_WhenSymbolWithNumberLengthGreaterThanPrecision"), | ||
| new TestCaseData(3, 2, true, "00.00").Returns(false) | ||
| .SetName("False_WhenIntPartWithFracPartGreaterThanPrecision"), | ||
|
|
||
| new TestCaseData(17, 2, true, "0").Returns(true) | ||
| .SetName("True_WhenNumberLengthNotGreaterThanPrecision"), | ||
| new TestCaseData(4, 2, true, "+1.23").Returns(true) | ||
| .SetName("True_WhenPositiveSymbolWithNumberLengthNotGreaterThanPrecision"), | ||
| new TestCaseData(4, 2, false, "-1.23").Returns(true) | ||
| .SetName("True_WhenNegativeSymbolWithNumberLengthNotGreaterThanPrecision") | ||
| }; | ||
|
|
||
| private static IEnumerable<TestCaseData> IsValidNumberScaleTests = new TestCaseData[] | ||
| { | ||
| new TestCaseData(17, 2, true, "0.000").Returns(false) | ||
| .SetName("False_WhenFracPartGreaterThanScale"), | ||
| new TestCaseData(17, 2, true, "0.0").Returns(true) | ||
| .SetName("True_WhenFracPartNotGreaterThanScale") | ||
| }; | ||
|
|
||
| private static IEnumerable<TestCaseData> IsValidNumberPositivityTests = new TestCaseData[] | ||
| { | ||
| new TestCaseData(3, 2, true, "-0.00").Returns(false) | ||
| .SetName("False_WhenAcceptsOnlyPositiveButGivenNegativeNumber"), | ||
| new TestCaseData(3, 2, false, "-0.0").Returns(true) | ||
| .SetName("True_WhenAcceptsAnyAndGivenNegativeNumber") | ||
| }; | ||
|
|
||
| private static IEnumerable<TestCaseData> IsValidNumberSymbolsTests = new TestCaseData[] | ||
| { | ||
| new TestCaseData(3, 2, true, "a.sd").Returns(false).SetName("False_WhenGivenNotDigits"), | ||
| new TestCaseData(17, 2, true, "").Returns(false).SetName("False_WhenEmptyStringGiven") | ||
|
There was a problem hiding this comment. Давай добавим чуть больше данных для проверок - null, спец.символы - /r , /n, просто символы %$# и т.д. Число с каким-то символом? Символ с каким-то числом? Строка из пробелов? Что будет, если разделитель не точка, а запятая? А если разделителей несколько? А если в строке только разделитель? Как валидатор ведет себя с очень большими числами? С очень большой точностью? А все вместе? Умеет ли он понимать числа в экспоненциальном формате? А может, валидатор считает верным число в другой системе счисления? Иррациональные числа? Возвращает ли валидатор на один и тот же входной параметр один и тот же результат? Для этого можно использовать атрибут А может, валидатор как-то меняет свой стейт? Что будет если подать сначала одно число, затем второе, а затем снова первое? Кстати, для того, чтоб обозначить, что метод ничего не изменяет (что-то внутри себя / входные параметры), то его можно пометить атрибутом Тесты как документация кода должны отвечать на все эти вопросы и даже чуть больше) |
||
| }; | ||
|
|
||
| private static IEnumerable<TestCaseData> ConstructorArgumentExceptions = new TestCaseData[] | ||
| { | ||
| new TestCaseData(-1, 2, true).SetName("WhenPercisionNotPositive"), | ||
| new TestCaseData(1, 2, true).SetName("WhenScaleGreaterThanPercision"), | ||
| new TestCaseData(1, -1, true).SetName("WhenScaleNotPositive"), | ||
| new TestCaseData(1, 1, true).SetName("WhenScaleEqualsPerci sion") | ||
| }; | ||
|
|
||
| [TestCaseSource(nameof(IsValidNumberPositivityTests))] | ||
| [TestCaseSource(nameof(IsValidNumberPrecisionTests))] | ||
| [TestCaseSource(nameof(IsValidNumberScaleTests))] | ||
| [TestCaseSource(nameof(IsValidNumberSymbolsTests))] | ||
| public bool IsValidNumber_Returns(int precision, int scale, bool onlyPositive, string number) => | ||
| new NumberValidator(precision, scale, onlyPositive).IsValidNumber(number); | ||
|
|
||
|
|
||
| [TestCaseSource(nameof(ConstructorArgumentExceptions))] | ||
|
There was a problem hiding this comment. Найс. После рефакторинга видно, что источник данных для теста с конструктором слишком далеко от самого теста. Давай протащим Да и в целом история хорошая, когда данные лежат как можно ближе к месту использования. |
||
| public void Constructor_ThrowsArgumentException(int precision, int scale, bool onlyPositive) => | ||
|
There was a problem hiding this comment. Возможно, стоит добавить тест, когда конструктор не кидает исключений. |
||
| Assert.Throws<ArgumentException>(() => new NumberValidator(precision, scale, onlyPositive)); | ||
| } | ||
|
|
||
| public class NumberValidator | ||
| { | ||
| private readonly Regex numberRegex; | ||
| private readonly bool onlyPositive; | ||
| private readonly int precision; | ||
| private readonly int scale; | ||
|
|
||
| public NumberValidator(int precision, int scale = 0, bool onlyPositive = false) | ||
| { | ||
| this.precision = precision; | ||
| this.scale = scale; | ||
| this.onlyPositive = onlyPositive; | ||
| if (precision <= 0) | ||
| throw new ArgumentException("precision must be a positive number"); | ||
| if (scale < 0 || scale >= precision) | ||
| throw new ArgumentException("precision must be a non-negative number less or equal than precision"); | ||
| numberRegex = new Regex(@"^([+-]?)(\d+)([.,](\d+))?$", RegexOptions.IgnoreCase); | ||
| } | ||
|
|
||
| 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) | ||
| return false; | ||
|
|
||
| if (onlyPositive && match.Groups[1].Value == "-") | ||
| return false; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,83 +1,101 @@ | ||
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
| using System.Net; | ||
|
|
||
| namespace HomeExercises | ||
| { | ||
| public class ObjectComparison | ||
| { | ||
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Category("ToRefactor")] | ||
| public void CheckCurrentTsar() | ||
| { | ||
| var actualTsar = TsarRegistry.GetCurrentTsar(); | ||
|
|
||
| var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70, | ||
| new Person("Vasili III of Russia", 28, 170, 60, null)); | ||
|
|
||
| // сравнение родителей такое же как и в коде, который нужно было отрефакторить - сравнивает по ссылке, | ||
|
|
||
| // а значит при создании экземпляра для сравнения, если родитель родителя будет не null, | ||
| // то будет ошибка, если это не ошибка в тесте, а так и должно быть, | ||
| // то реализация CheckCurrentTsar_WithCustomEquality работает неправильно, | ||
| // тк в таком случае она засчитает тест | ||
| // также данная реализация CheckCurrentTsar не сравнивает поле Weight для родителя, | ||
| // также как и оригинальный код, из-за чего данная реализация | ||
| // будет показывать результат отличный от CheckCurrentTsar_WithCustomEquality при | ||
| // небольших изменениях в TsarRegistry или в экземпляре expectedTsar | ||
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options | ||
| .Excluding(tsar => tsar.Id) | ||
| .Excluding(tsar => tsar.Parent)); | ||
| expectedTsar.Parent.Should().BeEquivalentTo(actualTsar.Parent!, options => options | ||
| .Excluding(parent => parent.Id) | ||
| .Excluding(parent => parent.Weight)); | ||
|
|
||
| // данная реализация лучше CheckCurrentTsar_WithCustomEquality тем, что: | ||
| // 1) при ошибке в тесте, выводится объяснение того, что пошло не так | ||
| // 2) код теста легко читается - методы записаны последовательно, и что они делают понятно из названия | ||
| // 3) название теста на прямую отражает происходящее в коде и соответствует выводимому результату | ||
| // 4) более удобная в плане расширяемости | ||
|
|
||
| } | ||
|
|
||
| [Test] | ||
| [Description("Альтернативное решение. Какие у него недостатки?")] | ||
| public void CheckCurrentTsar_WithCustomEquality() | ||
| { | ||
| var actualTsar = TsarRegistry.GetCurrentTsar(); | ||
| var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70, | ||
| new Person("Vasili III of Russia", 28, 170, 60, null)); | ||
|
|
||
| // Какие недостатки у такого подхода? | ||
| // 1) функциональность теста разбита на 2 метода, усложняя его читаемость | ||
| // 2) тест уже посути не сравнивает царей, а проверяет работу метода AreEqual | ||
| // 3) отсутствие пояснения - только выкинет "ожидается true, а было false" при ошибке | ||
| // 4) такая реализация AreEqual может вызвать переполнение стэка, тк сравнение родителей рекурсивно | ||
| Assert.True(AreEqual(actualTsar, expectedTsar)); | ||
| } | ||
|
|
||
| private bool AreEqual(Person? actual, Person? expected) | ||
| { | ||
| if (actual == expected) return true; | ||
| if (actual == null || expected == null) return false; | ||
| return | ||
| actual.Name == expected.Name | ||
| && actual.Age == expected.Age | ||
| && actual.Height == expected.Height | ||
| && actual.Weight == expected.Weight | ||
| && AreEqual(actual.Parent, expected.Parent); | ||
| } | ||
| } | ||
|
|
||
| public class TsarRegistry | ||
| { | ||
| public static Person GetCurrentTsar() | ||
| { | ||
| return new Person( | ||
| "Ivan IV The Terrible", 54, 170, 70, | ||
| new Person("Vasili III of Russia", 28, 170, 60, null)); | ||
| } | ||
| } | ||
|
|
||
| public class Person | ||
| { | ||
| public static int IdCounter = 0; | ||
| public int Age, Height, Weight; | ||
| public string Name; | ||
| public Person? Parent; | ||
| public int Id; | ||
|
|
||
| public Person(string name, int age, int height, int weight, Person? parent) | ||
| { | ||
| Id = IdCounter++; | ||
| Name = name; | ||
| Age = age; | ||
| Height = height; | ||
| Weight = weight; | ||
| Parent = parent; | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Раз
IEnumerable<>типизирован, то послеnewнет нужды ещё раз писать тип - можно его убрать. Компилятор сам разберется.