Many teams struggle with code reviews. They’re slow, they’re difficult, they lead to arguments, and worst of all they’re ineffective. After a while, everyone stops caring about them and the code review becomes a meaningless exercise. “It compiles…it doesn’t crash when I start the application…ship it.” Some teams stop the code review process altogether, perhaps not officially, but no one bothers to ask for one and no one bothers to complain when it’s missing.
But it doesn’t have to be like this!
At General Digital Software Services (GDSS), the code review is so central to our process and so ingrained in our approach to software that it has become unthinkable to skip the code review process. Our reviews are quick and easy, and they are extremely effective. We’d like to share some of the things we’ve done to get there, and hopefully they’ll help your team get there, too.
The most important piece of the puzzle is that our code review process doesn’t start when the actual code review starts. The code review process starts before a single line of code is ever changed. We generally think of this concept as “Code for Review.”
The basic idea is that the person doing the development work will give some thought to what these changes will look like to someone performing the review. It’s not enough for the changes to be accurate and correct. They MUST be easy to review, or as easy as possible, anyway. It’s a different way of thinking about the software process. It sounds like a lot of extra work for the developer, and in some cases it can be, but most of the time it’s just a matter of grouping like changes together and keeping dissimilar changes on separate commits.
For example, let’s say I have to go into a function that’s got a couple of bugs in it and also has become too large, thus, it needs to be split up. And maybe there are some incorrect comments that should be fixed up. The traditional way of doing this change is to simply fix the comment, split up the function and fix the bugs all at the same time. I can almost guarantee that this will make for a horrible experience for the reviewer, and the chances of finding a mistake will be much lower than it needs to be.
At GDSS, we would almost certainly split this up into three separate commits. The first commit would touch NOTHING but the incorrect comments. The second commit would only address the bugs, and we might even put them on separate commits if we feel just one will be confusing. The final commit will be to split up the function, and to the extent that it’s possible, the split up will be done with a strict cut and paste—no reformatting, no extra fixing—nothing. Just cut and paste. If more clean-up work needs to be done after the split, that would be an additional commit.
That’s a lot of commits, but it’s not really any extra work for the developer. All he or she has done is gathered up his work in logical chunks and maybe hit the commit button a few extra times. But to the reviewer, it’s heaven! Why?
First commit: comments only. I can look at the changes to see that they make sense, and then click “ignore comments” in my diff tool and see that only comments have changed. It probably takes me 20 seconds to do this.
Second commit: bug fixes. Again, if it’s just one logical fix per commit, this review should be very simple.
Third commit: function split up. It’s a cut and paste job, so using a modern diff tool, it should be very simple to figure out that the code hasn’t actually changed, and all that really needs to be reviewed are the new function call and parameters.
Of course, there will be reviews that are more difficult than this, but we’ve found that by implementing the principle of “Code for Review,” the vast majority of our code reviews become very fast and simple non-events. This is so important to us that we sometimes reject changes without even looking at them. “I can’t review this mess. Please redo it.” Yeah, it stinks but what else can you do? Fake the code review? If it can’t be effectively reviewed, it goes back.
Once you have a simple and effective code review process, it will no longer be a meaningless step that only gets done because you have to. Developers will come to embrace and depend on the process. “The team’s got my back.” That’s how it should feel, and that will only ever happen if the process is truly effective.
But Code for Review is just one piece of the puzzle. Here are some others:
Are you still using CVS/SVN/RCCS/etc.? Well, cut it out. Use a modern source control tool of some kind. At the moment, we’re using Mercurial. With a modern tool, the physical act of performing the code review becomes very simple. There are many different ways to do it, but our general workflow is:
- Developer makes changes and commits in their local repo, and runs “hg serve” to start the web server
- He’ll ask for a code review, and another developer pulls the changes into his local code review repo
- If the reviewer is happy, he’ll give the developer the OK to push to the central repo
It’s very fast and very simple. With due respect to my SVN toting colleagues, I can’t imagine how anyone could ever do quick and effective code reviews with such a thing.
Experiment with different diff tools. I have at least 5 or 6 separate diff tools on my system. Some are paid for…some are free. All of them are good at different tasks. Don’t be afraid to spend a couple of bucks on a quality tool. A painter needs a brush. Software engineers need diff tools.
And finally, take ego out of code reviews. By it’s very nature, it’s an adversarial process. This is where the team lead needs to step in and set the tone. Yes, people will find things wrong with your code. THIS IS GOOD! It should be expected that your code isn’t perfect the first time. If you’re not finding errors with code reviews, you’re probably doing it wrong. “The team’s got my back.” When you’re doing it right, that attitude is the natural consequence.