Programmers have many tools available to them for improving the quality of their code. One of my personal favorites is the code review — getting another set of eyes on my source code always challenges my assumptions, and invariably flushes out assumptions I’ve missed or mistakes I’ve made. I’d like to cover what I believe are the key elements to a successful code review. However, I would like to stress that what works for me may or may not work for you.
A good code review starts out by picking a good changeset to review. We don’t always have the luxury of performing a thorough review on all commits for all programmers due to various pressures of the job, so it is important to be picky about what gets reviewed. The trick to finding the right changeset to review boils down to “importance.”
- Is the code on the critical code path for the application?
- Is it a large amount of new code?
- Is it a large refactoring of old code?
- Is it particularly “tricky” code?
- Is the code from a junior programmer?
Obviously these are not the only criteria for picking a changeset to review, but I’ve found that these are a pretty good bet. Critical code paths need reviews because they are essential for the application. “Tricky” code needs review because of how easy it is to miss something important. Large amounts of new code or refactoring need review not only for correctness, but also maintainability. Reviewing code from junior programmers is always a good idea because it tends to have a higher bug count, and is also a fantastic opportunity for mentorship.
Once you’ve decided on a changeset to review, I think it’s best to perform an “offline” review of it. If you try to review the code for the first time with the author sitting right there, it tends to feel almost like you’re rushed. After all, while you are scanning their code, they just have to sit there and not get work done. By taking it offline, the other person has the opportunity to continue working while you perform a more thorough review of the code. The danger of doing this, however, is that the longer the time between the author committing the code and you performing the review with them, the greater the strain for the author to remember why they wrote the code the way they did. I call it “the pagefile problem.” As we work, any change in context causes us to page our current task to the swapfile in our brain so that we can start working on the other problem. These context switches are cognitively expensive, and also can suffer from brain bitrot. How many times have you had to stop working on some code for an hour or so, come back and lost your train of thought? Interruptions are a problem to avoid. So pick a time that’s conducive to both you and the reviewee to minimize the impact. I find its best to have a set schedule — perform the code review first thing in the morning, or just after lunch. But be careful not to pick times when people’s brains are going to be distracted. A Friday afternoon or just before lunch is never a good time to perform a review since people’s minds tend to be elsewhere, and there’s no time for them to implement any of your changes. But more on this later…
Doing your offline review is probably the most crucial part of a code review, but it’s also likely the most obvious part. You should look over the reviewee’s code for all the usual suspects:
- Bugs
- Coding standards violations
- Maintenance problems
- Test cases, or the ability to test
- In the case of refactoring, regressions
- Readability (this includes appropriate commenting)
These likely don’t come as a surprise because they’re what we usually think of when we think “code review.” But as you pour over the code, make notes (either inline via a code reviewing tool, or offline via something as simple as a text file or email message). Be sure to track not only where the issue is and what the issue is, but you may also want to track explanations as to why it’s an issue, links for further reading, etc.
One thing which I suggest you track that many people don’t always think of is prioritization. Not all code review tasks are created equal, after all! A crashing bug is a very important thing to get fixed. But a code portability issue or language foible may be less important. Ideally, the reviewee will fix all the issues found, but the reality is that they may not be given the chance to do so. Make it easier on them by pointing out the difference between “must fix” and “should fix.”
Having finished the offline review, the next task is to sit down with the author and perform the actual review. As I mentioned above, pick a time that’s conducive for everyone involved. While it’s not always possible, it’s ideal to pick a time where both you and the author are ready for a context switch, and where the author has the opportunity to make changes to the code before moving on to their next task. The start of the day or after lunch tend to work best because those are “ramp up” times. If you are doing it at the start of the day, the notes you took offline will be a big help to you since the review may not be fresh in your mind. I like to give the offline review to the reviewee prior to performing the in person review just so they can familiarize themselves with it. Usually only about 5-10 minutes before the review, just so they can see what’s coming and can plan their time accordingly (after all, a review with only one item won’t take the same amount of time as a review with three pages of items).
The in person review is actually quite important for a thorough review, but perhaps not for the reasons you think. It’s also the hardest part of the review process. Reviewing in person allows you to discuss each of the items found, but in a collaborative manner. Just because the reviewer thinks something is a problem does not make it so! The reviewee usually has far more contextual information than the reviewer, and some of the items found may not be issues at all. For instance, the commit being reviewed may be the first of many, and so the reviewee is already aware of things that need to change. Or the reviewee may not agree with the maintainability of the code, etc. At the end of the day, a code review is a partnership between the reviewer and the author, and neither one is omnipotent. So as a reviewer, go into the review expecting to be challenged and as a reviewee, remember that you have a say in matters as well. This is actually why the in person review is so difficult in some regards. As a reviewer, you have to be careful not to go into things on the offensive. Their code doesn’t suck, they’re not stupid and you’re not always right! And as a reviewee, you have to be careful not to go into things on the defensive. You’re not being attacked, and you have nothing to prove. This part of the review process is meant to be a discussion between peers (even if the author is a junior programmer and you have been a code guru for 20 years). It is your chance as a reviewer to explain where you think problems may be and ways to solve them, and it is the author’s chance to explain why they wrote the code the way they did. Effectively, every code review is a chance for you to exchange knowledge and so you should treat it as an opportunity to learn as well as teach.
Important things to cover during the in person review are:
- What assumptions were made by the author when writing the code?
- Why was code written the way it was? Is there a better way to solve the problem?
- In what way can the author improve the code? Why is that an improvement?
- What questions does the author have?
One thing I like to do during the in person review is to have the author explain their code to me. I find far more problems with their code this way than I do during the offline review. It points out where their assumptions are, it points out why the code was written the way it was, and most importantly, it gives you the chance to double-check that the code does what the author intends. I can’t count how many times I’ll be listening to an author describe what a piece of code does and I notice the code is subtly different from what they say. So encourage the author to explain their code to you in detail, without simply reading the code back to you. It will find things to ad, or remove from the list of work items.
At the end of the in person review, you should have a prioritized list of things for the author to fix. Most importantly, this list should be agreed upon by both you and the author. But this is not the end of the code review! Most importantly, the author should fix the items on the list and you should review their changes (in context) until everyone is satisfied. Without follow-through, the code review process is purely educational and not very practical! So be sure to follow up on your review. You can do this by adding review items to the bug database and tracking progress that way. Or it can be more informal where the author sends you an email asking you to check out a new changeset. Regardless of which way you do it, a code review is not complete until the list of items has been satisfied.
So that’s my take on code reviews and the process I like to go through. However, that’s just one programmer’s way of doing things! How do your code review practices differ? Do you have tips and tricks you like to use as well?
