You Should be Doing Formal Code Review
Published on September 30, 2010 by Jesse Storimer
The two practices that have contributed most greatly to my development as a programmer are TDD and formal code review.
Much like TDD, you should be doing formal code review. This isn’t for QA purposes, or because developers are untrustworthy in general. Much like TDD, the benefits aren’t just the outputs of the behaviour, the real benefit is that formal code review changes the way that you think about code.
From a managerial standpoint the most obvious benefit is that two heads are better than one. Much like pair programming, it’s best to have more than one developer look at every line of code that makes it into your product.
From a developer’s standpoint formal code review often looks like a nuisance. If your current workflow is to write tests, write code, and then ship it, formal code review introduces a brand new blocker into that simple chain of events. Now you need to wait for other developers to comment on your code, you need to resolve any comments they make, and make sure that everyone else is as happy with your code as you are.
Let me tell you, it’s well worth it.
Here are some reasons why:
It helps you disidentify with your code.
By disidentify I mean that it helps you think of your code as outside yourself. This may seem obvious but I know a lot of programmers who don’t make the distinction between code and self. The real point of this is that in order to produce the best code possible you need to be able to admit to yourself that what you already have may not be perfect.
Let’s face it, developers are often overly confident in their work, and telling them that something is done wrong can be taken as a personal attack. If you get used to letting other people look at, and critque, your code then disidentification becomes a necessity. This also goes vice versa, you need to be able to talk about the code of your peers without worrying about them taking your critiques as a personal attack. The goal here is to ensure that the best code possible makes it into your final release.
It makes you avoid shortcuts.
This one may not be true for everyone, but it’s true for me. Before ever doing formal code review I would often choose suboptimal solutions in order to get changes online fast and get on to the next thing. There was a good chance that other developers wouldn’t read my code, and of course the end users of the software wouldn’t know the difference.
But when you know, for a fact, that your peers will be reviewing every line of code that you write you will ensure your code is as good as it can be. It’s pretty motivating to think that your peers will be looking at, and commenting on, all of the little shortcuts that you took in your latest patch.
Really, it’s about being accountable to your fellow developers. Like I said, when you’re writing code for your end users they won’t know how pretty it is, how fast it could be, etc. But when you think of your end users as your fellow developers, motivation increases to ensure that code is readable, extensible, and optimized.
It encourages you to review your own code.
When making changes and pushing them directly to a production branch, there’s often little motivation to review your own code once you get all the tests passing. “That seems a good enough indicator that things are working, on to the next thing!”
Again, when you know, for a fact, that your peers are going to review this patch, you’ll be motivated to give it a thorough review yourself before anyone else does. I’ve found that this step is crucial in the development process, much like the refactoring step of TDD, during this step you will always find ways to improve on what you’ve already got.
Reading other people’s code helps you to improve, and helps you improve others.
This tip is often given by elite programmers: Read more code. Formal code review facilitates that. It’s not a matter of some programmers being better than others so that the n00bs can learn from the hax0rz, it’s the point that different developers work on different things and solve problems in different ways.
I’ve found that programmers can learn most when they are exposed to the programming styles of other programmers. This also helps to create a funnel effect in terms of your Coding style. Any organization will have an explicit (or implicit) set of Coding style guidelines. Formal code review will ensure that any code that is not in accordance with style guidelines gets noticed. Similarly, any code that introduces new, better, styles will also get noticed!
It makes for better code.
Formal Code Review Tools
OK, so now that you’re convinced you need to be doing formal code review, you need tools to support this. If your team is small enough you might think that simply emailing around review requests with Github links would be good enough, but I assure that there is a better way, especially if you have a team size > 2.
The tool that I am most familiar with is called Rietveld. The CLI and UI are less than perfect, but the overall workflow is great. You can see an example project running on App Engine.
My only complaint about Rivetld is that it was built for SVN and has poor integration with Git. See Gerrit.
Gerrit I am less familiar, I’ve only tried the demo. From what I can tell the UI is even more complicated, but the software itself seems cleverer and knows more about Git. I can’t speak to the workflow.
With Pull Requests 2.0 Github is becoming a viable platform for formal code reviews. My opinion is that their tools aren’t quite ready yet but they may be in the future. Missing features: inline commenting on diffs, ability to shorten the list of reviewers (you don’t always want everyone on the project reviewing changes), ability for individual reviewers to mark changesets as resolved, and a Merge! buton. With a few more features, Github really could be the best of the bunch.
There is often talk in the Ruby community of deploying code dozens of times per day, supporting Agile development or Extreme programming, realeasing early and often, etc. You hear less talk about code quality and maintainability, which are the two main benefits of formal code review.
For the reasons mentioned above, code making it into your project will be of higher quality and higher maintainability. The second point, maintainability, is especially important as the size of your development team grows. When the size of a development team gets above 3, and especially as it enters the double digits, code begins to move very fast. A lot of code will be committed to the project on any given day. Development at that pace is hard to track, and quality/maintainability is left up to the programmer writing the code.
By implementing formal code review you can spread some of that workload. Make sure that a new feature going into your project gets a thorough review by more than just the programmer who wrote it. Like me, you’ll come to enjoy reading and reviewing code almost as much as you enjoy writing it.