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.
3 comments:
Your 80/20 guideline can be useful, but not everyone will get to that payoff the same way. For example, if you keep track of where defects are found during code review (or have a tool do that for you) then you can eventually focus your reviewers on those sections of the code, and don't bother to review the rest.
Another option is to set guidelines for how much time each reviewer should spend doing reviews - we have a free book on code review that shows results of studies on that topic.
With respect to the "programming by proxy," some folks consider that investment a good use of time - as long as you don't burn people out. But in general I would agree with your point, and there are many of those sorts of things that can be detected by static analysis tools, so it's generally better to automate as much of that stuff as possible.
There are additional thoughts on best practices for code review on our web site.
Some good points there gsporar. Thanks for stopping by.
I would especially agree about the use of automated tools. A lot of stuff should not be part of human code review because a tool can find it automatically. This would include style violations like line width, spacing, tab vs space, the use of "final" as in the example, Javadoc violations.
With the use of tools then the human reviewers would be freed to just focus on real defects and the value of the code reviews would increase while finishing much faster with fewer iterations.
It seems that you're refering to developers who work for companies which sell their software. In those cases run-time errors are the most expensive things because, 1. you have to release new code, new patches, maybe a whole new exe. 2. You could lose you customers.
I don't work in that kinda shop. We write apps for ourselves. If there's a bug in the release, we don't have a 17,000 customer install base. We have a couple of dozen servers which need updating or a new smart client that gets pushed automagically on startup to a couple hundred users over a LAN.
While I agree with the premise, I disagree with the specifics of the conclusion. In our case, the worst criminal, is the inefficient code bug. We have developers that write queries that work fine in their 7 row test area, but will bring a 24CPU Unix box with Oracle 10gR2 EE to its knees on the 100 billion row table there. (slightly exaggerated for effect). This should never be allowed into production and should be caught via review.
The other assumption you make is that the programmer who wrote the offensive (SQL in this case) knew that it was wrong. That he would be embarrassed to let a reviewer see it and will write it correctly instead. First, there are a lot of pretenders in the business. They may have been great as an Access admin in their previous employer - the local candy store - but here in the Fortune 200 world, they ain't cutting it. The code review is about the only way to teach these neophytes the proper way. Second, your premise also assumes full-time employees. When you work in a large group of consultants from major firms like Sungard or D&T or Accenture, their success metric isn't the esteem of fellow programmers. There is no Programmer Mal-practice. He can write consistantly crappy code but as long as it satisfies the contract, he'll get paid, and move on to the next gig, with or without your respect.
I fully agree with your characterization of the problem and the solution of focused, specific reviews. I also believe that your specific guideline works for you and your company and might work for others with a business model that comes closer to your own; but in our model, that just isn't enough.
Post a Comment