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:

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:

  1. Back up your original changes before adding any Jester bugs to them.
  2. 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.
  3. 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.

  4. 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.
  5. 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:

  1. 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.
  2. 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.
  3. 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.
  4. 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?

12 Responses to “Bugging Your Code Reviews – See if your peers are really reading your code.”

  1. Micah Cowan Says:

    I’ve never been to a code review that wasn’t a bigger waste of time than anything it ever saved.

    You mention justifying your design decisions, but IMO if you wait until the design has already been implemented, it’s too late. It’s a big waste of time to reimplement a different design: if the design might conceivably need justified, then it should have to be defended before the first line of code has been written (this is dependent on how easy it is to change the design, of course).

    AFAICT, reviewers aren’t really looking for actual bugs most of the time, so much as they’re looking for stylistic problems that have a good potential to hide bugs (or, quite often, just doesn’t agree with their individual tastes). So, they might well miss the fact that that loop doesn’t have a guaranteed exit condition, or that your forgotten break statement wasn’t an intentional fall-through, but they’ll complain when you use a global, or hide names with an inner scope, or use a break statement instead of an extra variable for the loop condition (depending on the sensibilities of the reviewer).

    In my experience, though, they’ll find an awful lot more potential than actual problems, and frequently they’re of a purely stylistic nature. Humans browsing through code and guessing what it does/is supposed to do, isn’t nearly as effective as writing test cases around the code and proving what it does, including corner cases.

    Also, in my experience, an addition where a subtraction was meant, is usually easy to catch/find (depending on how frequently that code is executed, of course: another argument for test cases), whereas design problems (especially, lack of proper encapsulation, making it difficult to adjust how the program works later on) cause much bigger headaches.

    This is all just my 2¢; there’s nothing scientific about anecdotes/my personal experience. OTOH, I have yet to see scientific support for code reviews, either: it’s all theoretical. That’s sort of

  2. Micah Cowan Says:

    Sigh… seems like Firefox insisted on cutting my comment. The rest:

    That’s sort of my beef against Software Engineering practice: there’s all these paradigms that people assume are superior, because they sound like they could be superior, but no one tries to verify it.

    None of this helps you if you’re at a company that insists on code reviews, of course. But I’m not sure jester bugs will help you improve the reviewers’ abilities to catch bugs; more likely it’ll piss off the reviewer that you questioned their ability to catch bugs. ;)

  3. AlSweigart Says:

    Micah: I suppose “design decisions” was too high level of a description, I mostly meant justifying small implementation and algorithm details (but not so small where it becomes an argument of where the curly braces go).

    I agree that design decisions should be made and approved before the actual coding starts.

  4. Dougal Stanton Says:

    If you use a code review tool (like a web-based code review) then you could add the Jester bugs through that interface, so the tool knows the real code from the tainted code.

    This way you can also get the code review tool to check the validity of the review. Reviewers can mark a line they don’t like, like a game of minesweeper. If they submit and they haven’t marked a tainted line then it rejects their review.

    Damn that’s evil… ;-)

  5. Tony Says:

    Oh Dougal, that is simply brilliant! Then we could also have top-scores and ladders for reviewers who “play” the best… Until someone figures out how to build a bot based on the test-suite that’s supposed to accompany the software anyway.

  6. voladia Says:

    What a waste of time. We write software to push product out door to support business. If we delay push through petty actions, we increase our business’s expenses.

  7. AlSweigart Says:

    voladia: It is exactly that “push product out the door” mindset that causes people to skip doing real code reviews. At that point, you have no way of telling if they are being effective.

    The best way to keep costs down (and support the business) is to catch bugs as early as possible and BEFORE they are found by the public.

  8. Bob C Says:

    I think this is a brilliant idea, in that i aspire to be a shrink wrap software developer one day and have become the goto guy for code reviews. Unfortunately the mindset that voladia is tossing about, as well as the 80/20 rule are really prevalent here.

    I review two people code on a regular basis, both are back end apps and notorious for crappy adherence to standards or anything else for that matter, so i get my times worth.

    My question to you is… when i submit my code changes for review how do you think i should respond when i get a failure in response? How should i react when they come back the fourth time and have just refused to notice the semi-colon on the end of my if statement?

  9. Bob C Says:

    Also – Its important to realize that many things cannot be tested with Unit tests or even jester bugs being inserted. What about style or UI issues that are blatent that go ignored or unaddressed?

    You may not be able to automate everything away, as many people of the geek mentality appear to believe. I happen to be a huge proponent of using technology for what it is good for and find people trying to use it in incorrect ways abhorrent.

    Let me know your thoughts

    =) Huge fan and follower.

    You may appreciate the many responses to your videos from on my blog. Keep up the good, clear, rational thought process.

  10. Maintenance Man Says:

    I have heard of this idea before. And I think it is called Defect Injection. You basically game the code to be reviewed to see whether your peer review process is working.

    Recently I asked my manager if we could try this at my work. He was very supportive. Previously my project has done peer reviews with great success. However the practice has fallen to the side due to the normal pressuress of software development.

    Thanks for the post.

  11. Bobo The Sperm Whale Says:

    Throw in a multi-threaded race condition memory corruption bug for kicks! They’re like, sooo easy to spot too.

  12. Alexwebmaster Says:

Leave a Reply

free blog themes