Code Reviews
Mario and Mike posted their thoughts on code reviews, so I figured we should turn this into a full blown meme.
Have the Review
This is the most important step — far too much code sneaks through without any sort of review. If you know a code review is coming, I guarantee you will spend more time cleaning up your code.
Don’t be too Formal
One of the first projects I worked on many years ago had a formal code review process. We had specific forms to fill out, and huge lists of categories for things like “type of issue”. There were far too many choices, so nobody ever knew what to pick. In the end, having too many choices led to useless data. Plus, we all dreaded code reviews.
See it Run
I always ask the programmer to show the feature in action before we see the code, so we all understand what we are reviewing.
Break It
I try really hard to find ways to make the code fail. I’d rather find errors now than after deployment.
Avoid Duplication / Learn from Others
Without reviews, programmers work in relative isolation from each other. As a result:
- They don’t learn from their peers, because they may not be aware of other approaches to similar problems in the code base
- Code is duplicated, for the same reason
Having routine reviews is the best way I know of to ensure programmers really see and learn from each other’s code, and avoid writing the same thing over and over again.
Simplify
If I can think of ways to simplify code, I’ll offer those suggestions in a code review. Try to imagine looking at this code a year from now without the original programmer present. Like it or not, each line of today’s code will become tomorrow’s legacy code.
Angry pitchfork guy once told me: “Any code review can be hijacked and turned into a meeting exclusively about where to put the curly braces.”
Wheee!!!! It’s a meme: http://hamletdarcy.blogspot.com/2009/07/10-second-code-review.html
Good tools are a must here. You shouldn’t need to hold a meeting or print something out to have a code review. It simply won’t happen. You should be able to go back and forth several times on a review and review several changes per day. It should be a normal part of your flow. You need a tool that keeps track of which changes you need to review and that lets comment in line (i.e. click on a line of code and add a comment below it). We use Gerrit on Android: http://code.google.com/p/gerrit/ Google Code hosting also has good tool support. Atlassian’s Crucible is another option.
Eric lists “have the review” as the most important step which is absolutely spot on. Just knowing that your peers are going to inspect your work has a much bigger impact on your standards than most of us are willing to accept.
As a kind of an extension to the “learn from others” point. A big benefit that we’ve gained from continuous code review is a much more open and ongoing architectural dialogue. At a time when upfront design is out of vogue, code review threads are sometimes the only formal record of your choices (or rather the reasoning for your choices and compromises). It also provides the impetus to act on good ideas to change direction, which would otherwise get lost as random IM or hallway chats.
Unfortunately most people still think of code review as some PITA formal thing rather than a natural part of the day to day that everyone thinks of as constructive and necessary. I’m obviously biased (I work on Crucible http://atlassian.com/crucible) but I couldn’t agree more with Bob’s point that tooling up is critical. As well as Crucible and Gerrit there is Reitveld, Reviewboard and on the commercial front CodeCollaborator. They are all quite different, find the one that suits your team and toolset just make sure that your tool serves you rather than you serving it.