Excellence Through Code Reviewing

Do you do code reviews in your workplace? How often? What’s the experience like? I find for many programmers, code reviewing is not treated as a skill, but an annoyance, and that is a sad state of affairs.

The practice of code reviewing is indeed a skill that can be learnt and improved. It’s a skill that is vital part of a professional programmer’s toolkit. It’s a critical safeguard for deploying robust and maintainable code. Yet, it is constantly overlooked or marginalised as optional.

When I first started writing this blog post (months ago!), I downloaded a whitepaper from Smart Bear, a software company that creates a tool to assist with code reviews, called Best Kept Secrets of Code Review. In the whitepaper was an excellent quote about code reviewing that resonated:

“The reviewer and the author are a team. Together they intend to produce code excellent in all respects, behaving as documented and easy for the next developer to understand. The back-and-forth of code-and-find-defects is not one developer chastising another – it’s a process by which two developers can develop software of far greater quality than either could do alone.”

The justification for code reviews is simple: “given enough eyeballs, all bugs are shallow” (also known as Linus’ Law). No one can argue against reducing the number of bugs, right? Plus, as per the quote above, it won’t just be killing bugs, but increasing the quality of the software overall.

Those points alone should be strong enough, but on top of that there’s what I see as the “killer feature”: code reviewing is probably the best way outside of pair programming (and in some ways, it’s even better) for teaching and learning the craft of developing software.

If it’s so simply compelling, why is code reviewing overlooked? Probably because it sounds like such a trivial thing to do: I’ll look at your code and tell you if it has any issues. Put that way, it really is simple, but as always the magic is in the details. How much code should we review? How long should it take? How much should we “allow through”? How often should we review? The points of difference for code review approaches can be quite lengthy.

What are some of the problems with code reviews as practiced in many places?

  • They are cursory, seen as a speed bump on the way to being “dev complete”, and as such they’re not done thoroughly enough.
  • They’re only done by senior or lead developers as they’re seen as the only ones capable of reviewing code.
  • They’re avoided because they are too revealing, too invading, too scary.

And I’m sure you could think of more reasons too.

I don’t think they’re good reasons, and in my experience they’re actually easily defeated. If a team of programmers decide to change their culture to emphasise code reviews, it’s actually quite an easy thing to do. But it does require the intention to get started.

If we assume we can override the inertia of not doing code reviews, then we’ll want to know how to do good ones. Here is what I think makes up a good code review:

  • It is an iterative process, not a one off - by that I mean we shouldn’t wait for the “end of coding” to do the review. Have someone review the code at points throughout the progress on a task, not just at completion.
  • It should not be “cursory”, you need to invest some time in it (but not too much!)
  • A concise checklist can dramatically improve effectiveness
  • Code reviews are blame free zones
  • Code reviews are about learning, knowledge sharing, quality.
  • Code reviews are empirically better than not code reviewing.
  • Code reviews should be a simple process
  • Code reviews are assisted by checklists
  • Code reviews should be performed by everyone, not a select few

And here are a few notes on how to be a good code reviewer:

  • First off, pay attention to the list above!
  • Treat a code review as the important learning opportunity that it is - both for the reviewer and reviewee.
  • And most importantly: remember that code reviewing is a human process

What does a “human process” mean? It means that during a code review you are dealing with people, even as you inspect computer code. The code may be deterministic (hopefully!) but the individual whose code is being reviewed is as fallible as you or I. It pays to remember to keep it focussed on learning and quality, and remove blame from the equation.

Go forth and review! It really is as simple as that, and I can’t think of anything that will build an engineering culture and raise the standard of code produced by a team as quickly and effectively. As a little starter: remember that there are millions of lines of open source that can review and you don’t need permission, just go ahead and do it (and for some inspiration for the sorts of things that may be interesting to cover in a review, see Fabien Sanglard’s site for the many interesting and public code reviews that he has done.)