by Jim
Dec 10, 2009 12:26 PM
We’ve been having a little difficulty around the office with code reviews. We want to do them more often. There are all the usual conflicts about finding the time and charging the client for it, but additionally there’s a whole lot of confusion about what constitutes a code review.
So here are a few of my thoughts.
1.) Code reviews are about fixing code.
They can and should help the people involved learn from their errors, adopt better practices, and write better code. But what's the point of examining a piece of code and finding its weak points, and then leaving that bad code in your project? Fix it! Fix it now! Fixing the code while the code review is fresh in you mind will help make the review's lessons more concrete, and teach you how to implement any abstract concepts. It will help you remember what you've learned, for the next time you need it.
2.) Code reviews are not post-mortems.
This follows from point one. If you do a code review near the end of the project, when QA is well into their testing, then you can't go in and fix the code. Though the point is to make it better, you can always introduce new bugs. Thorough Unit tests greatly reduce the risk of refactoring, but it's always there.
Do code reviews early, so code can be refactored before problems propagate, and before QA has tested affected areas. If the code is off-track, finding and fixing the problem early reduces the effort needed to fix it, and greatly increases the likelihood that the fixing will get done. Management is not going to let you refactor the whole data layer when QA is almost done.
3.) A code review is a discussion, not a report.
A code review might find bugs, or clear violations of company coding policies, which are unequivocally wrong. But mostly it's about best practices and design principles. Very often there are pros and cons and underlying reasons, that aren't immediately obvious. And often these are matters of opinion. It's not about imposing rules and telling people what to do. It's about learning the reasoning, the "why," so that we can apply these lessons to new situations.
For this reason, a code review isn't just an enumerated list of things that were done wrong. It's a discussion about how everyone can improve their code.
And the more people in the room, the better, on both sides of the table. If we're going to talk about best practices, let's all benefit from it.