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 None
using 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 writex is None
."This advice is intended for the case where
x
could be an arbitrary value passed in by a caller, andNone
is being used as a sentinel value in your data structure. In this case, there's a risk thatx
belongs to a class with an__eq__
method that returns true for comparisons withNone
, resulting in a false positive.But in other cases, where the type of
x
is known, orNone
is not being used as a sentinel value, the advice does not apply. At best you might say that usingis None
is 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::string
data structure in C++ behaves like this.) In this case, accumulation by repeated concatenation leads to quadratic runtime. (And so in C++ you need to usestd::accumulate
.)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.