Вопрос по поводу проведения Code review со стороны тим-лида.
Где грань, что нужно отправлять на исправление, а на что можно просто забивать? Насколько высока должна быть планка качества?
Пример: был написан код (коменты мои):
$averageSumPerMonth = Orders::getInstance()->calculateAveragePricePerMonth(Auth::getAccountId()); // 15 or null
if ($averageSumPerMonth) {
$averageSumPerMonth = $accountService->getBalanceService()->getAmountString($averageSumPerMonth, 0); // '15 рублей'
}
Строка результата $averageSumPerMonth
далее передаётся в шаблон.
Так как кроме этого кода в методе (контроллере) ещё много подобного кода, мне кажется логичным вынести это в отдельный метод, чтобы также избавится от переписывания одной переменной значениями разных типов.
Достаточное ли это обоснование чтобы завернуть код или лишние придирки?
Я согласен, что никто не умрёт, если этот код попадёт в продакшен. Но хочется держать планку качества выше.
Основой вопрос - как понять не перегибаешь ли ты палку?
Насколько высока планка качества которую нужно соблюсти, чтобы пройти code review в вашей компании?
$averageSumPerMonth
используется не единожды и есть предположение, что то может измениться, то однозначно вынести, но только как то страшновато оно у вас выглядит ;). Если честно, но до конца понялтакже избавится от переписывания одной переменной значениями разных типов
, что вы тут имели ввиду? И возможно станет понятно из-за чего тим-лид завернул код...возможно не только мне... – borodatych 7 окт '15 в 8:46$averageSumPerMonth
можно сделать внутри и отдавать либоnull
, либо15 рублей
, имхо. – borodatych 7 окт '15 в 8:56также избавится от переписывания одной переменной значениями разных типов
- первый метод возвращает число а второй конвертит это число в валюту пользователя и возвращает строку вида 15$ или 300 рублей – Alexey 7 окт '15 в 9:05