As I mentioned in my recent post about how my team does agile, one of the core ingredients of our process is that nobody is allowed to check in without having gone through a code review and a test review. No other team that Ive worked on previously has had such a rigorous process around code reviews while weve had ad-hoc pair programming and occasional code walkthroughs, there were no rules about all code being reviewed before check-in. So when I first joined my new team at the SDC, I was unsure what to expect or if Id like it. But as you might guess from the title of the post, Ive become a convert.
First, let me go into a bit more detail about how the process works. A developer prepares a change set which normally consists of one completed requirement, or one or more bug fixes. Once they believe they are ready to check in, they will shout out Code Review!, at which time any other developer who can spare some time will volunteer to be the reviewer. In some cases the reviewee will seek out a specific reviewer if they know them to be best qualified in the relevant technology or component.
A code review will typically take around 15 minutes, but they may be considerably longer or shorter. It takes place at the reviewees computer (we normally have our entire team working in the same room. For a while we did have one developer in another location - in this case we mimicked the same computer approach by using desktop sharing and speakerphones). Normally there isnt any need to walk through the functional requirements or the high-level design in any detail, since the entire team is involved in the planning sessions and generally knows the critical details. However in some code reviews there may be some use of the whiteboard to communicate any design details that are needed to provided context to the code.
The review is performed by looking at the list of changed files in Visual Studios Pending Changes window, going through them one-by-one (sometimes from top to bottom, sometimes in whatever order the reviewee thinks is most logical), and performing a Compare with Latest operation on each file. Most of us have Beyond Compare or something similar installed to make this easier, but the Visual Studio text comparer works OK as well. We dont have a specific checklist of things that need to be reviewed, but some typical areas for discussion include:
- Quantity and quality of unit test coverage
- Code readability, method and line length
- Opportunities for reusing existing functionality, or merging new code with existing functionality
- Naming conventions
- Consistent application of patterns
- Globalisation (appropriate use of culture classes, resource files etc.)
- Hacks, shortcuts or other smelly code
If the reviewer is happy with everything in the change set, its ready for a test review (or if that happened first, its ready to be checked in). Alternatively, the reviewer can veto the check-in and insist that any uncovered issues are fixed first. In rare cases the reviewer may allow the check-in even if there are known issues, with TFS bugs or tasks created for tracking purposes. This option is most commonly used when the issues are minor and there are other developers waiting for the check-in before they can complete their own tasks.
So why did we choose to impose this additional workload across the development team? Well, its certainly not because the developers make a lot of mistakes or need close supervision the team is as experienced and capable as any Ive worked with. And in fact it is quite rare for a code reviewer to veto a check-in I dont have hard statistics but it probably only happens 1 time out of 10. Nevertheless I think the process extremely valuable for the reviewer, the reviewee and the quality of the codebase. First, each developer writes code with full knowledge that it will be scrutinised, so they take extra care to follow established patterns and avoid ugly hacks. Second, it helps share the wealth around who understands different parts of the solution. And finally it provides a very personal way for developers to learn from one another, whether it be a new Visual Studio shortcut key, a .NET API they didnt know existed, or a new architecture, design or testing technique.
One more interesting observation about how this process works in my team: at our retrospective meetings that we run at the completion of each iteration, there have been a number of occasions where people have called out that it takes too long to check in code. However Im not aware of any situations where anyone in the team has suggested abolishing (or even streamlining) the code review or test review processes to achieve this outcome. And having the support and confidence of the team is the ultimate measure of the success of any process.
