A
A
Alexey Solodky2015-10-06 19:26:24
PHP
Alexey Solodky, 2015-10-06 19:26:24

Where is the line of good enough reason to reject a Pull Request in a Code Review?

Question about the Code review by the team lead
Where is the line between what needs to be sent for correction and what can just be ignored? How high should the quality bar be?
Example - the code was written (my comments):

$averageSumPerMonth = Orders::getInstance()
   ->calculateAveragePricePerMonth(Auth::getAccountId()); // 15 or null
if ($averageSumPerMonth) {
   $averageSumPerMonth = $accountService->getBalanceService()
     ->getAmountString($averageSumPerMonth, 0); // '15 рублей'
}

The $averageSumPerMonth result string is then passed to the template.
Since, in addition to this code, there is still a lot of similar code in the method (controller), it seems logical to me to move this into a separate method in order to also get rid of rewriting one variable with values ​​of different types.
Is this a sufficient justification to wrap the code or extra nit-picking?
I agree that no one will die if this code gets into production. But I want to keep the quality bar higher.
The main question is how to understand whether you are going too far.

Answer the question

In order to leave comments, you need to log in

1 answer(s)
A
Andrey Nikiforov, 2015-10-06
@eoffsock

My opinion: if this is a hotfix - ok. But it should be noted that in the future this piece needs to be redone.
Similarly - if this is legacy support and in the future it will simply be thrown out.
In other cases, you need to rewrite so as not to run into code maintenance in the future.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question