What Code Review Should Not Find
I write so much about code review that I may forget to emphasize the obvious.
First, “code review” means different things to different people. I mean only that
Someone, who knows good code when s/he sees it, reads the code and gives the code author helpful comments.
Second, code review is not a goal — only a means to an end. The goal is better code now, and better developers as time goes on (which means better code in the future).
That said, I can get to today’s point, which is:
Code review is expensive, so use it wisely.
Do Not Use Code Review
Do not use code review to ensure that fixed text, such as copyright notices and authorship headers, are correct. For those, create code file templates and use them.
Do not use code review to find text layout problems. Instead, adjust your code editor so that it helps you lay the text out according to your style guide, and only lets you lay it out that way. In fact, skip the style guide — write a code editor configuration file, and perhaps some macros, instead.
Do not use code review to find where the language has been used incorrectly or unclearly. The person who wrote your compiler did that work already. Just turn the compiler warnings all the way up, and maintain zero compiler warnings. (And as Gerard Holzmann says in his rule #10, “The rule of zero warnings applies even when the compiler or the static analyzer gives an erroneous warning: If the compiler or analyzer gets confused, the code causing the confusion should be rewritten.”
Do not use code review to look for functional errors. Write unit tests instead. Not only is the CPU faster than a reviewer, and never gets bored, but the activity of writing unit tests often helps the code author see functional errors him/herself.
Do not use code review to find misuse of the language where the construct is error-prone. Consider the costs and benefits of moving to a language more suited to your environment, product, and developers, and if benefits outweigh costs, migrate to the better language.
Finally, do not use code review to find more instances of a problem if root cause analysis of past failures has already identified the problem as serious and systemic. Correct the problem first.
What’s Left for Code Review?
After all of the above is being addressed, then the valuable time of your code reviewers can be invested identifying readability, maintainability, and extensibility issues. For the most part, it takes a person to identify these issues. The reviewer is standing in for another developer—the one who will have to work on the code next.
These issues are the ones which affect how long it takes to fix, change, or extend the code, and how often fixes are required. The time saved by code that addresses these issues is what your time investment in code review is supposed to return, hopefully several times over.
Talk About Quality 
“Do not use code review to find misuse of the language where the construct is error-prone. Consider the costs and benefits of moving to a language more suited to your environment, product, and developers, and if benefits outweigh costs, migrate to the better language.”
I disagree. Moving to a different language is not always (almost never?) feasible. Developers should learn how to use a language/environment safely. Not every feature need to be used, and many features need to be used with care. Code Reviews are a great platform for teaching developers safe coding in any given language by example.
Lidor
Lidor
February 12, 2007 at 2:23 pm
While I don’t think the feasibility of moving to a different language—perhaps for a specific task or module—is so rare, I did not mean that one should always be looking to switch languages.
And if “teaching developers safe coding practices” in the given language is easy, and puts a stop to the errors, then by all means do it.
But if code review keeps finding misuse of the language because it’s hard to use that part of that language right, and yet it’s the only way to do it in that language, then one should consider “change programming language” as an option to escape the problem. No point just finding the same problem over and over in code review.
Tom Harris
February 13, 2007 at 12:46 am
[…] WordPress.com automatically added a link to an early 2007 post by Tom Harris on code review. As that post read reasonable and his blog even features a whole category on the code review, I […]
Are plugins that a time-saver in development? « Arnold Funken on the Social Web
June 26, 2008 at 7:40 am
I agree that human review should never be used where automated tools suffice. This rule also applies to code review checklists (if you use them).
On the review of functional problems, it’s true that unit tests remove the obvious problems. One interesting review technique I’ve seen is to review ONLY the unit tests. The idea is that if they are good (e.g. complete but not over-specified) it almost doesn’t matter what the code looks like because it’s easily changed.
This idea is probably too extreme — unless you’re going to review only a subset of your code changes, in which case it’s an interesting way to get pretty good bang for buck.
Jason Cohen
December 15, 2008 at 8:38 pm
From further experience, I would now clarify here:
1. It’s writing the unit tests, much less than running them, that finds the obvious problems. And we can’t get too carried away — it only finds obvious and simple problems, because more complex problems involve multiple units or components so require broader tests.
2. Keep code review on the deliverable code — depending on the unit tests to represent the code is removing oneself too far from the product. Code review is hard enough without that extra complication.
Tom Harris
November 16, 2009 at 11:19 am
Why “too extreme”? For projects where there are good unit tests, it sounds like a pretty good idea!
As for code review checklists, I have a study here somewhere on my desk that showed, contrary to popular intuition, that checklists don’t really help. Oh — here it is — it’s the respected and precise Les Hatton in his recent article Testing the value of checklists in code inspections:
Actually it’s not so surprising — the abilities and knowledge required for productive code review cannot be conveyed by a checklist to someone who doesn’t yet have them, any more than you could turn a beginning developer into an experienced expert with a checklist.
Tom Harris
December 16, 2008 at 10:46 pm