The same problems come up over and over again in code reviews and it can get a bit tiresome wheeling out the same basic items of advice. If it were a code review at work, you'd be quite within your rights to be a bit terse when the same problem appears for the tenth or hundredth time: "don't use
goto"; "always check the return value of
malloc"; "always compare against
is", "use a property, not an attribute"; "don't accumulate a string by repeated concatenation".
But here at Code Review most programmers are new to the process and so it's important to justify your advice, and explain why it applies to the original poster's code. Telling people to blindly follow rules doesn't help make them better programmers, and it doesn't help them spot the exceptional cases where the rule doesn't apply.
A couple of examples:
In Python: "don't write
x == None, always write
x is None."
This advice is intended for the case where
xcould be an arbitrary value passed in by a caller, and
Noneis being used as a sentinel value in your data structure. In this case, there's a risk that
xbelongs to a class with an
__eq__method that returns true for comparisons with
None, resulting in a false positive.
But in other cases, where the type of
xis known, or
Noneis not being used as a sentinel value, the advice does not apply. At best you might say that using
is Noneis a good habit to get into, because it might help you get it right in the situation described above.
"Don't accumulate a string by repeated concatenation."
This advice refers to the case where concatenation of two strings has to allocate a new object and copy both strings across. (The
std::stringdata structure in C++ behaves like this.) In this case, accumulation by repeated concatenation leads to quadratic runtime. (And so in C++ you need to use
But the conditions are not always true. For example, CPython implements an optimization that makes accumulation of strings by repeated concatenation efficient, so the advice does not apply. At best you might argue that the programmer might later want to port their code to other versions of Python that don't implement the optimization, and so it would be wise not to depend on the CPython behaviour.
Just in case it's not obvious: a justification needn't be lengthy: something simple like "don't write
x == None, write
x is None, because
x might have an
__eq__ method" would be fine, and sometimes there's a good answer on Stack Overflow that can be linked.