-
Notifications
You must be signed in to change notification settings - Fork 282
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
Зубакин Ренат #241
base: master
Are you sure you want to change the base?
Зубакин Ренат #241
Conversation
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.
еще мелочь, скорее как рекомендация, а не замечание, потому что задача очень маленькая, но в будущих задачах старайся придерживаться этих best practice
когда у тебя в солюшене тысячи классов, навигироваться ты будешь не по Solution Explorer, а по именам и неймспейсам (стандартное сочетание ctrl+shift+n и ctrl+n), и best practice тут такой: необходимо каждый класс выносить в отдельный файл, благо решарпер/райдер позволяют это делать в пару кликов (а так же лучше если имя класса будет совпадать с именем файла)
еще тесты обычно выносят в отдельный проект:
в тестах обычно присутствуют рефы на библиотеки которые нужны только для тестов, таким образом, отделяя тесты от логики, очищается intellisense и бинари боевого кода от тестов и всего что с ними прилетает, помимо этого уменьшая размер артефактов
cs/HomeExercises/ObjectComparison.cs
Outdated
* Изменения нужно будет вносить только в случае если мы хотим убрать какие-то поля из проверки. | ||
*/ | ||
actualTsar.Should().BeEquivalentTo(expectedTsar, options => | ||
options.Excluding((IMemberInfo o) => o.SelectedMemberInfo.Name == "Id" && |
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.
а если поле переименуют?
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.
что будет если в проверяемых объектах Parent будет ссылаться на корень графа?
@@ -12,7 +12,7 @@ public void Test() | |||
{ |
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.
лишнее то можно удалить
[TestCase(3, 1, true, "-0.0", TestName = "When_OnlyPositive_Is_True_And_Value_Is_Negative")] | ||
public void IsValidNumber_Should_Return_False(int precision, int scale, bool onlyPositive, string value) | ||
{ | ||
var numberValidator = new NumberValidator(precision, scale, onlyPositive); |
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.
этот тест и тот что ниже по сути одинакове, валидатор лишь разное возвращает, можно из метода возвращать значение и примитивы проверять силами NUnit
в TestCase можно задать еще и возвращаемое значение
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.
так же когда объеденишь у тебя будет много спецификаций в testCase'ах в одной куче (я бы еще добавил сюда кейсов, подумай еще какие могут быть кейсы), подумай как их можно сгруппировать
подсказка: помимо TestCase есть еще инструмент задать >1 кейса
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.
еще в догонку к комменту выше:
у тебя в этом тесте и в том что ниже несколько параметров одинаковые на все кейсы, может нет смысла передавать в тест некоторые параметры, а вместо них передавать другие?
@el_razor