Simple Code Review Checklists

What if, when giving a patch r+ on Mozilla’s bugzilla, you were presented with the following checklist:

You could not actually submit an r+ unless you had checked an HTML check box next to each item. For patches where any of this is irrelevant, just check the box(es) – you considered it.

Checklists like this are commonly used in industries that value safety, quality, and consistency (e.g. medicine, construction, aviation). I don’t see them as often as I’d expect in software development, despite our commitments to these values.

The idea here is to get people to think about the most common and/or serious classes of errors that can be introduced with nearly all patches. Reviewers tend to focus on whatever issue a patch addresses and pay less attention to the other myriad issues any patch might introduce. Example: a patch adds a null check, the reviewer focuses on pointer validity, and misses a leak being introduced.

Catching mistakes in code review is much, much more efficient than dealing with them after they make it into our code base. Once they’re in, fixing them requires a report, a regression range, debugging, a patch, another patch review, and another opportunity for further regressions. If a checklist like this spurred people to do some extra thinking and eliminated even one in twenty (5%) of preventable regressions in code review, we’d become a significantly more efficient organization.

For this to work, the checklist must be kept short. In fact, there is an art to creating effective checklists, involving more than just brevity, but I won’t get into anything else here. My list here has only four items. Are there items you would add or remove?

General thoughts on this or variations as a way to reduce regressions?

About these ads

About Josh Aas

Josh Aas is a Gecko platform software engineer with Mozilla Corporation.
This entry was posted in Mozilla, Programming. Bookmark the permalink.

11 Responses to Simple Code Review Checklists

  1. jgraham says:

    That seems like a reasonable idea; maybe when Review Board is finally rolled out we can add it as a feature. But if you wanted to make it effective there would have to be a less C++-specific list of concerns; to people developing javascript or python code having to click through to say that you have considered null checks just encourages clicking through the whole list without actually considering it.

    • Josh Aas says:

      This is just an idea I’m tossing out, not something I’m ready to start pushing for in our production systems quite yet. If we were going to do something like this, then you’re right – we’d want to tailor the checklist based on patch language detection or something like that.

  2. If we do that, the list will grow and grow and grow, and reviewers will check off the items as a matter of course because “I always check those things”, and I don’t think anyone is better served.

    I’m not super familiar with “industries that value safety, quality, and consistency” but I have a passing acquaintance with disciplines like ISO 9001 and CMM, and as far as I can tell they emphasize diverting resources to paperwork that says you did the right thing without actually making any improvements to what you do.

    If we can invest in improving the effectiveness of reviews we should invest in automated patch-checking tools instead:
    a) simple automated tools for checking style rules. This will free reviewers from having to think about those rules, which means they’ll spend more time thinking about more important issues.
    b) complex automated tools for finding actual bugs of the sort you’re concerned about.

    • Josh Aas says:

      I’m all for automated checking of this stuff – it would definitely be better than checklists. I’ve been hearing talk about it for a decade now though, progress has been slow. Checklists have the advantage of being technically easy to implement.

      You’re right, keeping them small would be a big challenge. Adding something to the list, especially if it isn’t replacing something, would have to be considered a really big deal. We’d also want an item cap (five items?).

      As for your comment re: ISO 9001 and CMM, the industry checklists I was thinking about are more along the lines of a pilot’s pre-flight checklist, or surgical pre-op checklists.

      Thanks for the feedback. I’m not really going to push for this right now, just wanted to throw it out there. I wish we’d have more conversations about the effectiveness of code reviews. Wins there do so much for us down the line.

  3. smaug says:

    The most time consuming reviews are the reviews for patches which implement some API from a spec. That requires one to find first bugs in the spec (specs are just untested pseudo code), file spec bugs, perhaps agree with the spec author how the issues should be fixed, and then finally check what the patch does and find issues in it.
    So if we had a checklist, it should certain contain an item “the API makes sense”, and
    a check box for the patch author might be good too. “yes, I have reviewed the API I’m implementing”

    For coding style and null checks we really should have tools doing the checks automatically.

    • Josh Aas says:

      I think it’s important to draw the distinction between a checklist that is meant to prevent mistakes, and a checklist that is used as part of an accounting system. I’m talking about the former, not the latter.

      Such a checklist should only include things that are relatively frequently overlooked, and should not include confirmation of basic responsibilities like “did you actually review the patch”. That’s tedious and unnecessary.

      If you’re reviewing a new feature based on a spec then reviewing the spec itself is either assumed to be part of your job or it is assumed to have been taken care of previously. It seems unlikely that a reviewer in that situation is going to completely disregard the issue of the spec being implemented. Also, the issue of spec review doesn’t come up often enough to warrant inclusion, imo – limited space on the list should go only to issues that are either very serious or very common.

  4. martin says:

    A) I typically check different things in different parts of the code so maybe the checklists should be “per directory” like .gitignore files? B) I check different things depending on who wrote the code; over time you learn which ppl make which kind of mistakes. C) I think I prefer if checklists were a “reviewer personal” thing and not centralized.

  5. Gijs says:

    The list should be very different for front-end patches, and like roc, I’m not sure for how long it’d help, and at what point it just becomes an obstacle (‘oh right, I need to check all the checkboxes’) rather than something of a memory aid of what you should be looking for.

    I think pure text in bugzilla reminding you of what to look for would be just as effective as actually requiring ticking of checkboxes, and less labour-intensive.

  6. davidb says:

    Scoping somehow to new_engineer_only and/or an honour system based opt-out might be good. Oh and a11y+Ux come to mind as criteria ;)

  7. kats says:

    I can think of a bunch more things but i agree that the checklist should be kept short. How about randomly picking 4 items from a longer list each time they are displayed? Not only does that keep people from getting desensitized and mindlessly clicking through the checkboxes, it’ll get people thinking about more aspects of the code over time.

    https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Reviewer_Checklist

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s