Five Reasons Why Structured Code Reviews are Waste of Money

For many people peer code reviews are the best thing to ensure quality when developing software. It produces a lot of documents and hence, that is what I think, project leaders love it because they have proven quality assurance activities. Let me explain why I believe that classic code inspections became unnecessary nowadays, how they can lead to inefficiency and which alternatives you have.

1. It is expensive!

A structured code review is a real process. A typical one, as it had been also used at Google until six years ago, looks like this:

1. Developer needs to list his code changes, usually and hopefully with the help of a tool (e.g. creating a patch file).
2. Developer looks for a reviewer. Either in person or by creating an online review ticket.
3. Reviewer fills a form and lists his findings. Each review must be well documented, with appropriate tools, in order to ensure that all code changes has been reviewed and findings can be understood.
4. Developer reads instructions, might discuss with the reviewer about some findings and implements changes.
5. Reviewer verifies changes.
6. Developer needs to check whether there are new findings. Eventually he or she submits the code.

A more formal process, according to Michel Fagan’s code inspection, will even have an assigned moderator, requires that all persons appoint at least one meeting and that every finding must be classified.

Take the rates of your developers and you can easily calculate the costs of a single review process. Needless to say that at least two of your developers cannot work on other tasks while they are busy with reviewing.

2. Code quality is subjective

(Often times) code is written by humans. Everyone has its own programming philosophies and interpretation of quality. I once worked with a guy who hated the increment operator in Java (e.g. i++), another one wanted to have attributes rather than elements in a new XML structure and then there was this debate about whether we should use an external library instead of having a helper method which consisted of ten lines of code. Arguing about this kind of stuff can be really tough and highly inefficient.

3. Wrong instrument for enforcing coding standards

Coding standards, code guidelines or style guides should be established in every project and can actually make your code more readable to the whole team. They try to solve high priority problems caused by wrong line intents, position of brackets and the number of parameters used in a method. And I have got the impression that the main purpose of most of the code reviews is to ensure that everyone is programming according to the guidelines. Simply because it makes it easy for the reviewer to list some findings. Everything else would be subjective anyway, as I already explained above.
However, this task can be done and should be done by our computers! They are much faster, cheaper and more reliable than us. There are plenty of appropriate tools for all major programming languages out there. And they do not bother when we think that they are sometimes a bit narrow-minded.

4. Only few people know how to master the art of accepting and giving criticism

When telling someone to do something in a different way than he or she did, then it is nothing else but criticism. And not everybody understands the good or bad power of it. While regular moderated feedback sessions are great, like the retrospective in Scrum can be, too often critics about the programmer’s most beloved task might just destroy the good team spirit which should be maintained in order to deliver high-class products. Consider this especially when your team is very heterogeneous and composed of youngsters, oldsters, “difficult personalities” and self-appointed rock star programmers.

5. No Replacement for Testing

Always when I read articles about how great peer code reviews are, such as “Effective Code Reviews Without the Pain” by Robert Bogue, the authors claim that the main purpose is to minimize defects. However, if it is mainly about the defects rate, I am wondering how much test code and test plans can be created for the same budget that you will need to execute structured code inspections. Finding possible defects by only analyzing the source code and not using the running application is very difficult.
Finding gaps often requires deep knowledge about the underlying use cases. It is not the responsibility of the reviewer, who is usually another developer, to verify that an implementation matches all requirements. In many cases he or she simply does not have the appropriate skills. Preventing defects is clearly the purpose of testing. Well written unit tests are much more reliable than a human debugger.

When it makes sense

To get me right, I am not against code reviews in general, but about the integration of code reviews in the daily development process with the goal to have the whole project code to be reviewed at least once. If you have the time and money, occasional reviews certainly do not harm. There are cases when you should definitely have a proper review session, such as

• A new programmer enters your existing team. He should get deeper reviews within his or her first month. I am sure that every project has its undocumented specialities.
• For junior developers code reviews are probably the best way to coach them. Invest in them, it is worth it!
• Critical APIs of modules which are intended to live a long time should be reviewed by at least one senior developer.
• When you do not know the developer’s skills, i.e. working with off-shore partners or in open source projects.

Alternative Approaches and Tools

• Use a different approach: Every code change must be quickly explained to a team member before it is getting committed to a code repository. The commit comment must contain the colleague’s initials, like “Seen by IH”. The actual goal is not to let the team member review the whole code, but that the developer identifies possible issues by himself/herself when talking about his/her own design decissions.
• Use static code analysis tools, such as Checkstyle or FindBugs, and integrate them into your Continuous Integration (CI) build. A sample process is explained at the Sonar blog.
• Enable a light-weight and unbureaucratic way to let your team check code commits. Have a look at Atlassian’s Crucible or Review Board.
• Let critical modules and interfaces develop in pairs. Read James Shore’s “The Art of Agile Development” for how pair programming works and whether it really fits to your team.
• Hire developers who know what they are doing! Basically, you get what you are paying for.

Scheduling time for quality assurance is a very good first step towards the right direction. However, rather than doing unnecessary code reviews, I would rather recommend to invest this time in well educated software developers, setting up a proper CI environment and having a real test management.