Wikipedia says Code Review is:
systematic examination (often as peer review) of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers' skills.
I like this definition, especially the part about improving the developers' skills. I think you really benefit from having your code reviewed by an expert in that particular area or a more experienced developer, but I digress. Actually I'd like to know how code reviews fit into the agile world.
Let's take a look at Google's process for managing changes:
- author edits changes in workspace & tests
- author sends email to reviewer
- reviewer views diff
- reviewer sends email back
- possible email discussion
- when reviewer gives it's OK (lgtm) authors submits changes
There are two things, which I think are very interesting about this process. First off, in terms of agile development, Google has in some sense implemented a strong Code Ownership model. On the one hand you are allowed to make changes to any class or module. However, before those changes actually go into the global code base, they need to be reviewed by the original author - or therefore owner. That's exactly what I think is the best model of code ownership. The second issue, which I find particularly interesting is the strong workflow of the process. Google tries to ensure quality, by forcing a review before any changes go live and - at the same time - improving the skills of their developers.
I like this processes a lot and I'm asking myself how could I integrate this with Scrum? Unfortunately I couldn't find much information on agile development and code reviews in general. Wikipedia brings up Extreme Programming and their own way of code review: Pair Programming. I prefer Google's way of implementing reviews - I'm not a big fan of pair programming.
There was a discussion on the Java Posse Google Group a couple of days ago, where someone suggested an explicit review task per sprint. Only reviewing a big junk of code at the end of a sprint is not only too late, but also no fun for the reviewer. In my opinion, the notion of a discussion and the (almost) immediate feedback by a co-worker is the most valuable part of a code review. I might be wrong, but I guess putting it off into a separate task is not ideal.
So where do code reviews fit in? If I'm not wrong in the world of Scrum, a task is finished if the code is ready to be release. That also means it has been tested and documented and thus testing and writing documentation are both part of the task. Why not make a code review part of the task? In fact, I think it should be the next step after your test passed and your code is ready to be checked in. I wonder if this could work.
Google's process is supported by their homegrown tool Rietveld, which is also available as OS version. It handles diffs, let's you annotate code with your comment and sends emails back and forth between the reviewer and the author. We have Atlassian's crucible at work, which basically works the same way. I like the tool a lot and I'm planning on using it extensively, as soon as the process of when and how to do code reviews is defined. I think it's important to have a good tool, that assists you throughout the review process. Nobody wants to scan through ugly unified diff files in order to copy and past code snippets into emails. It's just so much more convenient to use a tool.
I really wonder why there is so little information about code reviews in the agile world. It almost seems like there is no place or time for it (except for the crippled version used in XP), when you are doing agile. Maybe people are afraid of blaming each other. It might be tough for some developers to speak their minds, if they think something is wrong with the code the are looking at. There might also be the notion that it is a tool to blame other people, which I think the opposite is the case: you are helping out other developers to improve their skills. But you could also see it as a tool to make lazy developers think, before they check-in their code.