Motley says: "Quick peer reviews are sufficient. Team reviews? Waste of time."
Summary
Motley: Quick ad-hoc peer reviews are sufficient to nail bugs early in the development cycle.
Maven: A more structured team-based code review process can lead to more issues found.
______________________________
[Context: Maven overhears a code review that is happening in Motley's office]
Maven: Hey Mot. Were you and Marvin just having a code review? I overheard a little bit of it while you were at your desk.
Motley: Yeah, we just kicked butt - found a few nasty little problems, although nothing too significant due to my superior coding abilities.
Maven: I expect nothing less! I was just wondering what your process is for code reviews. Can you enlighten me?
Motley: Not really a process per se - we want to keep things nice and light. Code reviews are known to be an effective defect prevention technique so of course we leverage them prior to checking in code. When I am ready to check-in, I call over a fellow developer, I pull up a quick WinDiff display of the code that has changed, we do a quick over-the-shoulder check at my desk. I then fix whatever bugs we identify, and then I am good to check-in. We usually catch a few things on a code review, particularly when I review someone else's code. I am not a senior dev for nothing!
Maven: I applaud your use of code reviews to keep quality high! But-
Motley: I knew it! There's always a "but" with you. For your information, I've been doing some reading on code reviews and I think that given the amount of effort we are spending on the review relative to the number of bugs that we find, we are being effective. We focus on the diff, fix all the bugs on-the-fly, and move on.
Maven: Don't get me wrong - I think what you are doing is great. For many check-ins involving trivial changes that process can work. What do you do for more complicated check-ins like a few files of new code?
Motley: Same thing. It just takes a little longer.
Maven: Do you typically find more bugs than for other reviews?
Motley: I would say we find about the same number per thousand lines of code.
Maven: And do you still find problems later on in the testing cycle?
Motley: Of course! You can't expect us to find everything in a brief little code review. I still fee like we're effective though.
Maven: Informal, less structured peer code reviews as you describe are great for the following situations:
- Small bug fixes
- Minor code changes where the author is already very familiar with the code
- Changes of very low complexity
- Changes with few possible side-effects
More formal, structured, team-based code reviews are more suitable in these situations:
- Large bug fixes
- Scenarios where a lot of new code is being added to the system (e.g. a feature check-in)
- The first bit of code a developer on the team writes
- Changes on a trust boundary - i.e. security-related changes
- Complicated code changes, such as those involving concurrency
Motley: Whoa! You used a really bad word. The word "formal" really scares the crap out of me. Whenever some one uses "formal" to describe a process, look out. Those processes typically have high overhead and occupy a lot of time for - in my experience - little gain.
Maven: I hear you. In fact, I often agree. By "formal" here, I mean slightly more structured than just calling someone over to your office to quickly look at a bit of code. There is a process, but it's very agile and lightweight.
Motley: You also said "team-based". Why would I want to involve more than one person in a code review? That seems like a waste of time!
Maven: Studies show that team-based reviews that follow some kind of process - even lightweight - are the most effective defect finding techniques in the development cycle. You want to add people to your review to the point of diminishing returns. The ideal number is somewhere in the area of 3-4 reviewers. A more structured review tracks metrics over time that can help you determine how many people to include. It's all about being as effective as possible while not expending too much effort to get the job done.
Motley: More eyes, more bugs. I'm not sure I totally buy it. Why did you say previously that one of the scenarios where we should practice a more structured review is on the first bit of code a developer writes?
Maven: There is great value in that. The idea is that you get a few developers on a new project to review the code in detail for someone contributing their first few lines of code. A team-oriented review is a fantastic learning experience that gets the developer going in the right direction from the start. You see, humans are creatures of habit - if you make a mistake in your first few lines of code you are likely to repeat it in all your code. Fixing all of those problems expends wasted effort later.
Motley: Seems reasonable, but I could do that with just that developer and me.
Maven: You won't find near the amount of issues as a team of people. Use the technique sparingly, however. Microsoft, for example, could never team-review all 50 million lines of Windows Vista. You want to do team review in intelligent situations.
Motley: What else makes a team review different than an ad-hoc peer review? Right now I can't believe it would be that much more effective. And does this different kind of review process have a name?
Maven: A more structured review is typically called an inspection. Let's talk about it over lunch. To whet your appetite, here are some of the key principles behind an effective team-based code review:
- A structured, repeatable, but lightweight process
- Diligent individual preparation at a reasonable preparation rate
- Non-confrontational meetings to ensure "threats" to the author are removed
- A checklist reminder of the most important issues to look for
- Metrics that help gauge the effectiveness of your meeting
- A focus on process improvement to help prevent the same bugs from occurring again
Motley: My interest is slightly peeked. Where do you want to go for lunch?
Maven: How about-
Motley: Never mind - I'll pick. You drive.
______________________________
Maven's Pointer: The term "inspection" has been used throughout software engineering history to denote a more formalized code review. Invented by Michael Fagan at IBM in the late 1970's and used by many companies, inspections typically yield 45-75% (Code Complete 2nd Edition) of the defects injected into software throughout the development cycle. There are several different inspection processes, but the one we will focus on here has roots in the Team Software Process (TSP) with some modifications for efficiency.
Maven's Resources:
- Peer Reviews in Software, A Practical Guide, by Karl Weigers, Addison-Wesley, ISBN: 0201734850, December 2001.
- Code Complete 2nd Edition, by Steve McConnell, Microsoft Press, ISBN: 0735619670, June 2004.
