In most of the jobs I've had peer code review was an essential and regular part of the software development process. My experience is that it improves code quality and is well worth the effort. I'd also say that at least half of what I learnt in that time was through code review. Either someone would suggest improvements to my code, or I'd discover new ways of doing things from reviewing other people's work. It's a great way to share knowledge. This is not only true in specialist domains where the answers are not always easily found by searching the web, but any time someone is not aware of a better way of doing things. You won't search for something if you don't know it exists. That's my opinion, but there is also plenty of evidence that code review improves code quality, helps find bugs early and ultimately saves money. The numbers vary between studies, but finding bugs early is not just cheaper, it's significantly cheaper.

What about testing?

No doubt some people will say that the purpose of unit testing is to find bugs early or, in the case of TDD, prevent them ever being created. So why do we need code review as well?

I agree that unit testing is powerful, indeed I've used TDD thoroughly to tame some seemingly intractable problems, but I still highly value code review. I think testing and code review achieve different things. Both are important.

Unit tests are great for quickly checking that everything that worked previously still works after a recent change. They provide confidence to refactor or experiment with code in the knowledge that the essential functionality can be quickly checked and rechecked. Achieving this via a code review would be slow, boring and error-prone.

Code reviews on the other hand can ask bigger questions, like:-

• Is the code understandable?
• Are the unit tests testing the right requirements?
• Is there a more efficient way to do this?
• Does it increase unplanned technical debt?

In fact, any check that can't be automated. Coding style, static analysis, spell checking etc can all be automatic and the most a human reviewer should do is check the results.

Code review suggestions

Others have already written good things about how to conduct code reviews without annoying people or what tools you should use, so I won't repeat that. Here are my observations and suggestions about how to get the most out of code review:-

• The most important thing for a code review to check is whether the code is functionally correct. Does it do exactly what it says on the tin? Are the code and tests fully implementing the requirements of the story/ticket? Or has the developer misunderstood what's needed in some way? If so then they've probably written the unit tests with the same misunderstanding, so they all pass fine. Yes, this is a hard thing to check - it takes some time to fully understand, but it is important. Get this wrong and nothing else matters.
• Code review is a great way to share knowledge. Again, this does mean you have to take the time to properly understand the change, but it develops the team and mitigates risk.
• There should be no reviewer hierarchy. Different perspectives are useful. More than once a more "junior" developer has reviewed my code and asked a "naïve" question only for me to scratch my head and say, "You're right, I've done that totally wrong". Even when it doesn't happen quite like that, reviews tend to stimulate questions and the transfer of ideas.
• That said, it can be worth making sure the local domain expert is one of the reviewers of any important change, but they don't have to be the only reviewer.
• Applying more engineer hours to the review does find more bugs, but each additional hour will be slightly less valuable on average than the last, so there's a balance to be struck.
• Don't get bogged down in matters of opinion. When it comes to how to make code understandable and maintainable, there's a big slice of judgement involved. "I wouldn't do it that way" is not sufficient justification for raising a comment. There's a danger of getting into "tomayto/tomahto" arguments. Be open-minded, take a step back and think about how important your point is.
• Following on from the last point, it's OK to have no comments. It might seem like you haven't done your job as a reviewer, but it's better than nitpicking for the sake of feeling productive.
• If possible, do not manually check anything that can be automated. Style guidelines should be agreed across the project and checked quickly and automatically. Reviewers are human beings; they're above that stuff and anyway they're not very good at it.
• Pair programming can function as a kind of instant review, but there are also risks. The reviewer can be too "close" to the code to be impartial or may make the same false assumptions about the requirements. A slower or more timid engineer might not want to question the other's work without the time to think it through properly. So, while pairing can find bugs very early, I don't think it should be the only kind of review a change receives.

Similar posts

• 

  Should you change careers to become a software engineer?

• 

  Explaining technical debt - an analogy

• 

  How to write a great bug report