(I originally posted this on my MSDN blog.)
Formal Code Reviews
Formal code reviews are well-known as an effective tool for reducing bugs and improving the quality of your software. There’s lots of literature out there that demonstrates that the cost of finding and fixing a bug goes up exponentially the later into your product lifecycle it gets. Code reviews are by far the most efficient way to improve the quality of your code. Unfortunately the way code reviews are usually implemented on traditional software development teams doesn’t translate well to agile teams and usually turns out to be counter-productive.
Here’s how the reasoning for code reviews usually goes:
Premise: It’s useful and important to have multiple people look at a piece of code because another person may easily see and correct flaws that the author missed.
Secondary Premise: Without a formal process, it’s not likely that anyone except the author will ever look at a given piece of code.
Conclusion: we need a formal process to ensure that multiple people look at each piece of code.
Code reviews are most useful in the kind of traditional engineering environment where Dev A owns Feature A, Dev B owns Feature B, and they rarely cross the boundaries to look at each other’s code in the course of doing their jobs. Sometimes it’s even considered rude to make changes to code in someone else’s feature area. If you don’t have a formal process for getting multiple sets of eyes on a piece of code in that kind of environment then sure, it’s quite possible that no one besides the author will ever look at that piece of code. Or if they do look, they’re not going to strongly criticize it and they definitely won’t clean it up if it has problems.
So what’s the problem with formal code reviews? Well, in practice there are two problems. First, if you have a formal code review system that says people should get their code reviewed after they check it in then it’s probably not going to happen. It’s a pain and people conveniently “forget” to do it. Its the first thing to get pitched overboard in a schedule crunch. Don’t even bother because it doesn’t work.
Ok, so to fix that you can make the code reviews a prerequisite for checking in. Microsoft product groups use this strategy a lot and often establish elaborate rules that say you must get sign-off from a code reviewer before you can check in to the repository. This effectively guarantees review of 100% of your source code, which is great, but it causes another (huge!) problem.
Frequent Check-ins
Agile teams have found that frequent check-ins are a very important part of the agile development process. By frequent I don’t mean once a week or even once a day. I mean once an hour or even more. Every time you add a small piece of new functionality (ideally defined by a set of tests and the code that makes them pass), you should check in. If it builds cleanly and doesn’t cause regressions, check it in. What are you waiting for? This has several benefits:
- It promotes the spirit of “continuous integration” where the whole team is working on a single shared reality rather than individual copies of reality that need to be merged later. It prevents misunderstandings and makes it easy to get the latest code.
- Smaller checkins makes it much easier to find and fix a build break or to roll back a mistake.
- It gives the team a sense of momentum and accomplishment.
- It gives project managers good visibility into daily progress and how the team is doing.
We should do everything we can to promote frequent check-ins. Frequent check-ins are good for traditional teams but they’re absolutely critical for agile teams. There’s lots of literature out there on the benefits of checking in frequently.
Unfortunately a formal code review process works directly against frequent check-ins. I don’t want to interrupt my co-workers 10+ times a day to look at my stuff. I don’t want to interrupt my own flow to find an available code reviewer. Formal, pre-check-in code reviews simply don’t scale to an agile system of frequent check-ins. They’re incompatible.
(On a tangential note, I’ve observed teams where the check-in process is so laborious that they resort to using TFS shelvesets to pass around the “current” code to people who need it and it can take anywhere from a couple of days to more than a week for those shelvesets to get promoted to actual check-ins. People get confused about which is the most recent shelveset and code gets lost. This is a terrible way to work!)
XP to the Rescue
Going back to the reasoning behind formal code reviews, I strongly agree with the major premise. It’s critical to get multiple sets of eyes on every piece of code because it drastically reduces bugs and improves quality. However, it’s entirely possible to do software development in such a way that invalidates the secondary premise and largely eliminates the need for formal code reviews before checking in. It’s not true that formal code reviews are the only way to get multiple eyes on the code. There are multiple practices found in the Extreme Programming (XP) philosophy that help us do that in other, more efficient ways.
Shared Ownership
The first practice is to promote shared ownership of the code. Instead of creating a team of specialists where each dev has exclusive ownership of a feature area, we want to create a team of generalists where everyone has the flexibility to work on pretty much every area of the product. That doesn’t mean that we won’t have subject matter experts; of course some devs are going to be particularly knowledgeable about threading, or networking, or databases, or whatever. But that area expertise shouldn’t translate into exclusive ownership of a piece of code. The team as a whole owns the whole code base. The practical implication of this is that everyone on the team has permission and is expected to work on code they didn’t write and to fix it, refactor it, and clean it up when they do. This means that over time you’ll get multiple sets of eyes on most of the code which accomplishes the stated goal of a code review.
Swarming
The second helpful practice is to have the team “swarm” one or a few stories at a time rather than having each team member work on a completely separate story in parallel. This is in the same vein as the first practice; stories aren’t owned by individuals, they’re owned by the team. Not only that, but we want to encourage devs to actively work on the same stories at the same time so that when dev A checks something in, dev B will be reading, using, refactoring, and extending that code almost immediately. This does an even better job of accomplishing the stated goals of a code review.
Pair Programming
The third helpful practice is pair programming where you literally have two devs collaborating to write a single piece of code. People have referred to this (only partially tongue-in-cheek) as a “real-time code review”. Pair programming is a complicated topic and confers many benefits but yes, it entirely eliminates the need for formal code reviews before every checkin. This is the natural and logical conclusion when you follow the XP philosophy to its end.
Unfortunately, pair programming is not something you can simply slipstream into a team’s daily rhythm. It a radical philosophical shift in the way we do development and lots of people are deeply uncomfortable with it at first, for good reason. It’s a huge change and it’s possible to do it badly so that you don’t see any benefit from it. There’s lots of literature out there on how to do it well but it takes time and practice. If you’re transitioning a team from traditional software development over to agile development, you might consider starting first with shared ownership. That by itself can be a bit of a shock to people. Let your team digest that for awhile then try swarming, then pair programming.
Code reviews are one of the most powerful tools in a developers toolbox, but code reviews on an agile team probably won’t look the same as on a traditional team. Keep in mind the goal you’re trying to accomplish and look for ways to get there without sabotaging other important techniques.
SaintGimp 