Justify your review!

Question

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 write x is None."

  This advice is intended for the case where x could be an arbitrary value passed in by a caller, and None is being used as a sentinel value in your data structure. In this case, there's a risk that x belongs 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 x is known, or None is not being used as a sentinel value, the advice does not apply. At best you might say that using is 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 use std::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.

Answer 1

While I agree in principal with this post... I think there might be a really great solution to this.

Code Review doesn't really have duplicates, due to the nature of our questions. Without duplicates, it's hard for their to exist canonical answers. This is something we lack but Stack Overflow makes advantage of.

So, with that said, why don't we work on making some canonical answers?

We don't need to start closing things as duplicates all of a sudden, but consider this idea. One of the common repeated suggestions I see for C++ is:

Don't use using namespace std;

I don't actually know why. I'm not a C++ programmer, so I don't particularly care to know why. At this point, to me, suggesting people not use using namespace std; is essentially a meme.

It doesn't seem to contain using namespace std; so I don't know what to say.

A few months ago, I used to put warnings about using namespace std; everywhere too.

write a bot that checks new questions for using namespace std;, post generic answer, gain all the rep

What!? No using namespace std; ?!

if (using namespace std) { yell, scream and shout! }

Are you using using namespace std; ? If you are not, you are at least intermediate.

How hard can it be to say "Don't use using namespace std;"?

...and many others (plus tons of @CaptainObvious posts of people actually using it)...

I mean, tons of questions get posted with people actually using using namespace std;, yet apparently anyone who reviews C++ questions agrees it should either never be used... or the rare case in which it should be used basically never exists.

So... with all that said... someone who knows why using namespace std; shouldn't be used as people tend to use should:

• Find a good looking question which uses using namespace std;
• Post an answer to this question which only addresses this specific issue.

The answer should be very detailed, in-depth, and specific.

Now this answer becomes the canonical answer on using namespace std;.

When new questions are posted with a bad use of using namespace std;, answers can comment:

Don't use using namespace std;

And have a link pointing back to this canonical, in-depth, detailed explanation on why using namespace std; is bad.

Answer 2

I really don't feel like typing the same thing over and over. Sometimes I wish I could do away with the explanation, and be more like a teacher grading homework; this is wrong, this is wrong, this is wrong, this looks great.

Instead, I shape my answer to my mood and the asker's level of experience. Also based on the level of mistakes made. Sometimes I write whole pages.

What I'd wish for is that I wouldn't have to be a human lint tool. Maybe we could alter the help pages that a recommended practice is running a lint tool on your code before posting your question. The amount of times I note something like if(condition){ return true; } else { return false; } could be return condition is too many.

Answer 3

My take on this is that there's generally three different kinds of advice we can give:

1. The general, language-agnostic logic fixes

   Like the if (condition) return true; else return false example mentioned by @Pimgd, I don't usually over-explain these points. Short and succinct, unless the improvements can get quite significant (e.g. turning a convoluted if-ladder to fewer conditions, or a switch case).

2. The general, language-centric API/"best practices" fixes

   Sometimes, the OP may not be aware of certain libraries' APIs, and I will try my best to link to the more appropriate API. Of related note are those covering what-can-be-done-better improvements, as what I've termed as "best practices". A hypothetical example (in Java) would be someone looping through a HashMap using keySet() to iterate the keys, then do a get() to get the corresponding value and process the pair. The "best practice" way would be to just use the entrySet() method. For these, sometimes I'll just find a relevant SO link for more information.

3. Question-specific contextual fixes

   These are arguably the most interesting ones, because it encourages me to think from the perspective of the OP and see what do they actually want to achieve. Sometimes, this is related to the XY problem, and you realize that if you can get the OP to approach it from another angle, they might be able to do more with less. Alternatively, you discover that there is an edge case that the OP forgot to consider. I tend to explain these with a bit of tact, but sometimes I have to point out directly. I try not to short-change them in this regard. :)

In conclusion, I will only go to lengths to justify issues I spot under point 3 above... for the other two, I generally keep it short with supporting links.

Answer 4

I would agree, but with two "limitations":

1. You cannot possibly write out full/exhaustive explanations of why you recommend specific items.
2. Sometimes, specific individuals continue to ask for reviews of code that shows the same problems again, and again, and again.... do you need to explain the same recommendations each time?

Taking your string concatenation as an example, it is enough in an answer to say:

Building compound String values with an accumulator is generally better for performance than using direct concatenation.

Having the above as an answer would be fine, but it would also be fine for someone to 'challenge' that statement in, for example, a comment, and ask: "Why is an accumulator better?" (at which point a link to a Stack Overflow answer may be useful as a response?)

So, I agree that a great answer would have each suggestion "motivated" with an explanation of why a recommendation is made, but it is not necessary for it to be an answer, just for it to be a great answer.

As for the individuals who repeatedly use bad code practices, and refuse to adopt your suggestions, well, that is a little harder. My solution in the past has been to just ignore that user's questions. I have also, though, just ignored those issues in their code, and answered about other issues in their code (though that situation irritated me, and I won't do it again). Ideally you should be able to repeat all the same old suggestions each time, and create an answer that is 'self standing' and not dependent on other previous questions, and is still comprehensive. Unfortunately I find it hard to live to that ideal, so, instead of being cynical, mean, or sarcastic in an answer, I try to just skip it. I have not always been successful though.