-
Notifications
You must be signed in to change notification settings - Fork 1
01 leap year homework #4
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
base: master
Are you sure you want to change the base?
Conversation
| EXPECT_EQ(false, is_leap_year(1997)); | ||
| } | ||
|
|
||
| TEST(leap_year, test_1996) |
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.
название теста ни о чем не говорит. Из этого имени нельзя понять, что действительно разработчик хочет проверить
| bool is_leap_year(uint32_t year) | ||
| { | ||
| if (year % 4 == 0) | ||
| if (year % 4 == 0 && year % 100 != 0) |
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.
Да, зламався тест для 100
|
|
||
| bool is_leap_year(uint32_t year) | ||
| { | ||
| if (year % 400 == 0) |
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.
А тут після фікса теста для 100 поламався тест для 2000
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.
тогда ок
| return true; | ||
|
|
||
| if (year % 4 == 0 && year % 100 != 0) | ||
| if (year % 4 == 0) |
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.
При додані нового теста докостилювалися нові умови в if. Це хіба не підстава для рефакторинга?
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.
А до якого коміта прив'язані ці коменти? Бо в мене попередній коміт - це фікс теста для 4. І після теста я зарефакторив умову, тому що всі попередні числа ділились на 4.
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| bool is_leap_year(uint32_t year) |
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.
финальный рефакторинг не закончен, окончательный вариант функции +- вот таким может быть
return (year % 4 == 0) && (year % 100 != 0)) || (year % 400 == 0);
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.
это был бы последний этап - самый последний рефакторинг
No description provided.