Bugging Your Code Reviews – See if your peers are really reading your code.
Code reviews are the single biggest thing you can do to improve the quality of your code, as Jeff Atwood says. Code reviews force you to show your code and justify your design decisions to someone else, and the extra set of trained eyes can reduce a large number of costly defects. But how can you be sure that your code reviews are as effective as many claim them to be?
Of course, this is entirely dependent on if they are done right. I generally distrust the “bring the coworker to your desk and explain your code by talking at them while they nod” approach. Code reviews done right are the ones where your peer has to actually read the code and understand it for themselves.
I like the approach of zipping up the changed files and emailing them to the reviewer with a short summary of the changes. Not only is this a non-blocking operation that lets me go on to other work, but it forces me to write clear comments and documentation that will make sense to someone who does not have the luxury of asking questions to the original author. A good visual diff program to compare the changed source file to the one currently checked into source control lets the reviewer surmize the changes.
After a while, they email me back a few questions and suggestions or the code with changes of their own. Sometimes. Most of the time, they send me an email that looks like this:
Al,
Looks good. Check it in.
These emails are a nice ego boost and reinforce the idea that I am the world’s best programmer. But after a while, the neurotic paranoid in me began to wonder how close of an eye they were putting on my code. Especially after a few times where I noticed bugs right before I checked in the supposedly reviewed code.
So I came up the concept of Jester bugs. Jester is a unit test tester for Java. It works by making a syntactically correct change to the original source code under test, compiling it, and then running the unit test suite. If the unit test suite results in everything being A-okay, then you know that the test suite is flawed because it missed the monkey wrench that was thrown into the code.
Jester bugs are bugs that you intentionally introduce into your code, to see if your code reviewer is really spotting bugs in your code or just giving it a pass.
I’m open to suggestions, but I’ve nailed the process down to the following:
- Back up your original changes before adding any Jester bugs to them.
- Flip a coin. Heads means you add one bug, tails means you add two bugs. Having a predictable number of bugs will signal the reviewer at what point they can stop “really” looking for bugs in your code, so change up the number of Jester bugs.
- Add some bugs to your code. These bugs should produce runtime errors, and should not produce any compiler/linker warnings or errors. Some of my traditional ones are:
- Forgotten break statements in switch statements
- Infinite loops
- Adding completely unused parameters
- Substituting subtraction for addition (or vice versa)
- Get ideas for bugs from actual bugs that you’ve made that were spotted in previous reviews.
Change them up, and use appropriate bugs for whatever language you are using. A very subtle bug would be code comments that describe the opposite of what the code really does.
- Write down your bugs somewhere. I keep a work log that I type up my daily notes in. You want this recorded somewhere, so that you can see if all of the Jester bugs were found by the reviewer. Or so you can undo your Jester bugs if you lose your backup from step 1.
- Send your code to the reviewer.
If the reviewer comes back with saying everything is A-okay, then you know that the code needs to be reviewed again, either by that same person or by someone else.
The entire concept of Jester bugs is a bit sneaky, which is why its better to have it as a general policy instead of a unilateral decision by a developer. There are some objections to this practice, but I don’t see the merit in them:
- Adding bugs to reviews will slow down the review process – Not really. These bugs are the same things that developers should be checking for already. Adding the bugs to the code takes minutes, so the only way this process slows us down is by forcing reviewers to actually review rather than skim.
- Developers may accidentally check in code with Jester bugs – Also not really. Writing down the bugs before sending the code off for review can ensure that all of them are removed before checking them into source control. If Jester bugs are standard policy, then developers are less likely to forget whether they added them or not.
- Reviewers may have their feelings hurt by missing Jester bugs in their reviews – This attitude is what keeps many organizations from doing code reviews in the first place: developers don’t like to have their work criticized and torn down. But this unprofessional attitude is what keeps organizations from improving their quality in the first place.
- After a while, reviewers will know the common types of Jester bugs and be able to spot them, making the whole point moot – Great. Jester bugs are the identical the types of bugs that developers make by accident anyway. Spotting bugs is exactly what the reviewer should become more adept at.
What are your thoughts?
Leave a Reply