Козлов Данил #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,140 @@ | ||
| using System; | ||
| using System.Text.RegularExpressions; | ||
| using FluentAssertions; | ||
| using FluentAssertions.Execution; | ||
| using static FluentAssertions.FluentActions; | ||
| using NUnit.Framework; | ||
| using System.Collections; | ||
|
|
||
| namespace HomeExercises | ||
| { | ||
| [TestFixture] | ||
| public class NumberValidatorTests | ||
| { | ||
| public static IEnumerable IsValidNumberPrecisionTests | ||
| { | ||
| get | ||
|
There was a problem hiding this comment. Не совсем то, что я имел в виду. Про разделение Каждый новый кейс начинать отделять пустой строкой имеет смысл только, если кейсы сами по себе большие, но это не тот случай. Про разделение источников данных пустой строкой - это что после объявления Так что придется ещё немного поправить) |
||
| { | ||
| yield return new TestCaseData(3, 2, true, "+0.00").Returns(false) | ||
|
There was a problem hiding this comment. Зачем здесь |
||
| .SetName("IsValidNumber_ReturnsFalse_WhenSymbolWithNumberLengthGreaterThanPrecision"); | ||
|
There was a problem hiding this comment. Нет нужды и в названии теста, и в названии сценарии теста дублировать информацию. Можно и нужно упростить, оставив только нужную для понимания теста информацию. |
||
| yield return new TestCaseData(3, 2, true, "00.00").Returns(false) | ||
| .SetName("IsValidNumber_ReturnsFalse_WhenIntPartWithFracPartGreaterThanPrecision"); | ||
| yield return new TestCaseData(17, 2, true, "0").Returns(true) | ||
|
There was a problem hiding this comment. Было бы неплохо разделить пустой строкой кейсы с |
||
| .SetName("IsValidNumber_ReturnsTrue_WhenNumberLengthNotGreaterThanPrecision"); | ||
| yield return new TestCaseData(4, 2, true, "+1.23").Returns(true) | ||
| .SetName("IsValidNumber_ReturnsTrue_WhenPositiveSymbolWithNumberLengthNotGreaterThanPrecision"); | ||
| yield return new TestCaseData(4, 2, false, "-1.23").Returns(true) | ||
| .SetName("IsValidNumber_ReturnsTrue_WhenNegativeSymbolWithNumberLengthNotGreaterThanPrecision"); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Было бы неплохо разделить пустой строкой источники данных |
||
| public static IEnumerable IsValidNumberScaleTests | ||
| { | ||
| get | ||
| { | ||
| yield return new TestCaseData(17, 2, true, "0.000").Returns(false) | ||
| .SetName("IsValidNumber_ReturnsFalse_WhenFracPartGreaterThanScale"); | ||
| yield return new TestCaseData(17, 2, true, "0.0").Returns(true) | ||
| .SetName("IsValidNumber_ReturnsTrue_WhenFracPartNotGreaterThanScale"); | ||
| } | ||
| } | ||
| public static IEnumerable IsValidNumberPositivityTests | ||
| { | ||
| get | ||
| { | ||
| yield return new TestCaseData(3, 2, true, "-0.00").Returns(false) | ||
| .SetName("IsValidNumber_ReturnsFalse_WhenAcceptsOnlyPositiveButGivenNegativeNumber"); | ||
| yield return new TestCaseData(3, 2, false, "-0.0").Returns(true) | ||
| .SetName("IsValidNumber_ReturnsTrue_WhenAcceptsAnyAndGivenNegativeNumber"); | ||
| } | ||
| } | ||
| public static IEnumerable IsValidNumberSymbolsTests | ||
| { | ||
| get | ||
| { | ||
| yield return new TestCaseData(3, 2, true, "a.sd").Returns(false) | ||
| .SetName("IsValidNumber_ReturnsFalse_WhenGivenNotDigits"); | ||
| yield return new TestCaseData(17, 2, true, "").Returns(false) | ||
| .SetName("IsValidNumber_ReturnsFalse_WhenEmptyStringGiven"); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public static IEnumerable ConstructorArgumentExceptions | ||
| { | ||
| get | ||
| { | ||
| yield return new TestCaseData(-1,2,true) | ||
| .SetName("Constructor_ThrowsArgumentExceptionWhenPercisionNotPositive"); | ||
| yield return new TestCaseData(1,2,true) | ||
| .SetName("Constructor_ThrowsArgumentExceptionWhenScaleGreaterThanPercision"); | ||
| yield return new TestCaseData(1,-1,true) | ||
| .SetName("Constructor_ThrowsArgumentExceptionWhenScaleNotPositive"); | ||
| yield return new TestCaseData(1,1,true) | ||
| .SetName("Constructor_ThrowsArgumentExceptionWhenScaleEqualsPercision"); | ||
| } | ||
| } | ||
|
|
||
| [Test, TestCaseSource(nameof(IsValidNumberPositivityTests)), | ||
|
There was a problem hiding this comment. Атрибут |
||
| TestCaseSource(nameof(IsValidNumberPrecisionTests)), | ||
| TestCaseSource(nameof(IsValidNumberScaleTests)), | ||
| TestCaseSource(nameof(IsValidNumberSymbolsTests))] | ||
| public bool IsValidNumber_Returns(int precision, int scale, bool onlyPositive, string number) | ||
| { | ||
| return new NumberValidator(precision, scale, onlyPositive).IsValidNumber(number); | ||
|
|
||
| } | ||
|
|
||
| [Test, TestCaseSource(nameof(ConstructorArgumentExceptions))] | ||
| public void Constructor_ThrowsArgumentException(int precision, int scale, bool onlyPositive) | ||
| { | ||
| Invoking(() => new NumberValidator(precision, scale, onlyPositive)).Should().Throw<ArgumentException>(); | ||
|
There was a problem hiding this comment. Применение |
||
| } | ||
| } | ||
|
|
||
| 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.
Почему
public?