Why does code review fail?
That's a good question. Most people agree that code review is beneficial. It catches bugs very early when they are extremely inexpensive to fix. Knowing that the code is going to be reviewed keeps "crap" code out as the programmer will be far less inclined to try to sneak in bad code if he knows he's going to have to answer to it. Code review can identify and correct subtle defects that can be extremely difficult for the test team to produce in a lab testing environment outside the field.
I suspect a lot of the problem is when code reviews take on a life of their own. Code review is governed by the 80/20 rule and 80% of the benefit is realized in the first 20% of time expended. After that the point of diminishing returns is quickly reached and it becomes quite inefficient and unenjoyable for all involved.
So the trick is to set it up to get that 80% of benefit quickly, the easy wins, then basically halt the review when it reaches the point of diminishing returns.
Well then why do they drag out? What happens during that last 80% of the time. I think this is where software companies that start doing code reviews get into trouble. The reason reviews take on a life of their own is that the purpose of code review is not well defined. Some see code review as a time for mentoring, coaching, showing the "better way", being pedantic and showing off reviewer skill, training less experienced staff.
What that relates to is that "crap" code turns out to be extremely subjective. Here's some examples of stuff in Java that I've seen sent back in code review for rewriting.
int x = f1();
f2(x);
The reviewer sent it back stating that it be changed to
f2(f1());
Here's another
void f3(int p1, String p2) {
float f4 = ...
The reviewer sent it back stating that it be changed to
void f3(final int p1, final String p2) {
final float f4 = ...
This are examples of what I call Programming by Proxy. We have valid, working, maintainable code being sent back for rewrite. There's nothing wrong with the code as originally written, it works properly, the reviewer just wouldn't have done it himself that way so he demanded it be rewritten to the way the reviewer would have done it. Once programming by proxy becomes the norm then code review is well on the way to the trash heap of company history as a well meaning but failed experiment.
How to keep reviews on track
The answer to this is as simple as it is obvious. Impose discipline on reviewers. Specifically this: the only thing that is actionable in a code review is runtime errors. That's it, runtime errors only. Otherwise the code stands as written.
I know some people may at first recoil at that idea. Indeed it seems counter intuitive as you are losing the value of doing code reviews. But you aren't losing value. By focusing on runtime errors only you get the most value of the code review, positive changes to the code that will correct real user affecting defects. In the 80/20 rule 80% of the benefit of code review is elimination of defects. The rest is rewriting code that works which is of marginal benefit at best.
What about the crap code though? How do we keep that out if runtime errors are the only things reviewers are allowed to raise. There's a couple of counters to that. First of all by having to submit the code to review the programmer will be more inclined to check in good code if he knows for sure someone is going to see it. The way bad code gets in is when a programmer is working by himself with no guidance or oversight and never having to answer for his work; then he can become sloppy and take shortcuts. Simply knowing that the code will be looked at will prevent most of the crap code from getting in.
Also by utilizing the 80/20 rule the reviews will finish up so much faster. That way if the reviewer saw something in the review that he personally disagrees with (like the examples above), then the reviewer is free to go in there and fix it himself.
