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

Шмаков Данил #223

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

Conversation

Reltig
Copy link

@Reltig Reltig commented Nov 24, 2023

No description provided.

}

[TestCase(2, 1,"0.00", TestName = "zero intPart + fracPart should be less than precesion")]
[TestCase(3, 2, "+0.00", TestName = "zero intPart + fracPart + \"+\" should be less than precesion")]

This comment was marked as outdated.

@@ -7,26 +7,46 @@ namespace HomeExercises
{
public class NumberValidatorTests
{
[Test]
public void Test()
[TestCase(1, 0,"0", TestName = "int zero")]

This comment was marked as outdated.

[TestCase(3, 2, "a.sd", TestName = "non digit symbols")]
[TestCase(2, 1, ".0", TestName = "must have digits before point")]
[TestCase(1, 0, "0.", TestName = "must have digits after point (if exist)")]
public void IsNotValid(int precision, int scale, string validatingString, bool onlyPositive = true)

This comment was marked as outdated.


// 1. Вероятность StackOverflow из-за бесконечной рекурсии
// 2. При каждом изменении свойств или добавлении новых функцию придётся переписывать
// 3. Данный код не выполняет никакой полезной функции кроме сравнения полей двух объектов

This comment was marked as outdated.

// 1. Вероятность StackOverflow из-за бесконечной рекурсии
// 2. При каждом изменении свойств или добавлении новых функцию придётся переписывать
// 3. Данный код не выполняет никакой полезной функции кроме сравнения полей двух объектов
// 4. Нарушение SRP, ответственность за сравнение объетов должна быть инкапсулирована в класс
Assert.True(AreEqual(actualTsar, expectedTsar));

This comment was marked as outdated.

Copy link

@razor2651 razor2651 left a comment

Choose a reason for hiding this comment

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

еще мелочь, скорее как рекомендация, а не замечание, потому что задача очень маленькая

когда у тебя в солюшене тысячи классов, навигироваться ты будешь не по Solution Explorer, а по именам и неймспейсам (стандартное сочетание ctrl+shift+n и ctrl+n), и best practice тут такой: необходимо каждый класс выносить в отдельный файл, благо решарпер/райдер позволяют это делать в пару кликов (а так же лучше если имя класса будет совпадать с именем файла)

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

Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd"));
Action act = () => new NumberValidator(precision, scale, onlyPositive);
act.Should().Throw<ArgumentException>();

Choose a reason for hiding this comment

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

это можно проще написать через Assert.Throws<>( () => ...)

public void Test()
#region TestCases

public static object[][] numberTestCases =
Copy link

@razor2651 razor2651 Nov 27, 2023

Choose a reason for hiding this comment

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

такой подход неинформативен, из названия не понятно какой кейс проверяется пока не залезешь в код и не посмотришь что поменялось
image

во-первых: в TestCaseData можно указать имя, тем самым четко обозначив кейс
во-вторых: тут у тебя генерация кейсов на лету, это не очень наглядно с точки зрения спецификации, чтобы понять откуда кейс взялся придется дебажиться или изучать код

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

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