We’ve all seen them, and most of us make them. Process lists, to-do lists, lists of all kinds–there’s something very satisfying about checking a little box or crossing an item off a list. However, checklists can serve a greater purpose than as a physical reminder that we’ve accomplished something. In many fields, checklists exist because human brains are fallable and prone to distraction. Pilots use checklists to make sure that amid dozens of relatively simple tasks, none get forgotten. Checklists used before and during surgery reduced the death rate by nearly 50%.
I’ve been doing code review for a long time. In the open source world, code review happens on pretty much any third party contribution. In my time at VMware, pretty much every non-trivial change was expected to be reviewed before checking it in to source control.
In general, people are pretty good at the code review process, but it’s sometimes surprising what can slip through. A natural consequence of the way our brains look at the world is that it’s easy to pay a lot of attention to small details and code style flubs, and completely miss the big picture. It’s for this reason that a lot of designers will start with rough sketches when they start gathering feedback–if something looks too finished, reviewers will only give them comments about the details. Unfortunately, unless you’re starting out with pseudocode or architectural sketches, source code almost always looks “finished” and can therefore encourage some bad habits.
One of the ways that I avoid the pitfall of giving superficial reviews is with checklists. I have a bunch of different sets of checklists that I’ve developed and tweaked over the past few years, and for any given change, I’ll select the sections which apply. I’ve applied some of these at VMware, working on a large desktop application code-base (mostly at the user interface layer), as well as with Review Board itself.
Here are some examples of some of the many checklists I use in my day to day work:
User interface
Visual design (for a native desktop app)
Correctness (for C/C++ code)
Security (for C/C++ code)
Obviously, not everything is applicable for every change. If the review request isn’t making any changes to UI, then I’ll skip the first two checklists entirely. If a change is a bug fix, I typically won’t review it for architecture and design principles.
Put the big stuff first
Once I’ve figured out which of my checklists apply to any given change, I sort them to focus on the biggest stuff first. If I have major architectural objections with a new piece of code, it’s probably not worth reading through it looking for bugs or style guide violations. One of the worst things I’ve encountered (on both sides of the code review process) is working through a ton of small issues before realizing that everything has to be rewritten.
Working through the big stuff first also facilitates collaboration, because it’s clear at each stage how negotiable my comments are. Architecture can be argued about for a long time, and in the end a lot of it is a personal decision on behalf of the person writing the code. On the other hand, if there’s a lurking crash bug, it’s not an option whether or not to fix it.
Do checklists work?
I didn’t know what to expect when I first started applying checklists to my code review process. I anticipated that it would slow down my reviews quite a bit, but I didn’t know what the payoff was. I was pleasantly surprised to discover that it improved the quality of my reviews immensely, and after a few months of application, the amount of time I had to spend reviewing was almost the same as it was before.
My approach is to do a pass through the code for each and every item in the checklist. By only looking for a very specific type of defect, each pass goes relatively quickly, even for large changes. Focusing on only one type of defect allows an extreme focus that makes it trivial to find issues. One good example is scanning for (most) memory leaks: simply check each allocation for its equivalent deallocation, and verify that all code paths will go through that point. Even for large changes, a pass looking for memory leaks might take 5-10 minutes. Compare that to the last time you tried to chase down a leak after the fact, and the payoff is obvious.
In contrast, I find it incredibly difficult and error prone for me to read through code once looking for memory leaks and UI design issues and API usability and possible cross-platform build issues and everything else all at the same time.
I personally feel like when I pull out my checklists, my code reviews are much higher quality. My coworkers have also commented that they find my reviews to be pretty good, so I’m hoping it’s not imaginary.
Do you use checklists for code review? How are they working for you?
I run a somewhat unprofessional operation. I have a lot of code and it is not uncommon to introduce a bug in one spot while everything else is fine. I run a test suite that covers a lot, though not really. I try to make the rounds of testing everything, but sometimes it takes a month to discover a bug on the shipped version. I make an operating system with compiler. A lot can go wrong. I try to do a test install and an upgrade install in VMWare. I can’t test on native hardware, anymore. It is pretty unreasonable to test my install scripts when my work has mostly been changing a stupid game that is completely unrelated to install scripts! When I work on my compiler, all hell can break loose if I’m not careful, but I try to be careful.
This is gold. Awesome work.
We had talked about doing very similar things where I work. This resonates with some ideas that our team had put together. One concern though is remembering to use the appropriate checklist.
Is it possible to prepopulate a checklist of questions into the review on RB?
We participate in a bunch of student projects courses, and Felix Sung from MIT is currently working on building an extension that would let users create and use checklists inside Review Board. We can’t guarantee any kind of timeframe for when it will be ready, but as you might expect, this is something that I’m pretty excited about myself, too!
Is there an update on this checklist extension? The last information I have been able to find is 5 months old.
Felix’s project produced a prototype but was not actually usable. We have another student this fall working on taking things from prototype to production.
Good idea around having a CR checklist. Probably very helpful for people who are new to doing CRs. I think I appreciate the technical lists more than the very limited design questions.
If you are thinking about the design part of things during a CR, your process can probably be improved. CRs should be about code that implements existing designs/ideas etc and doesn’t actually introduce big things. However, sometimes edge cases are only surfaced when you are debugging/working on code, in wich case adding UI elements/user facing stuff that hasn’t been designed may make sense.
Oh, I just realized that you mostly work on OS.. in which I’ve heard nobody designs anything up front (or not much), just writes code – I can see these lists making sense in that context.
We do a lot of upfront design for new features, but a lot of the time we’re making incremental improvements to existing UIs. If we’re just adding a new check-box to an existing page, it probably doesn’t need a ton of design work (it’s faster to just write the code than it is to make a mock-up). Since I’m using these at the time of code review (as opposed to at design phase), those are the sorts of changes where I pull out my limited “visual design” checklist.
We have checklists. They work for us. They also have educational value, and they can also make code reviews easier, because you know what to look for, and people know what you are going to look for.
Great post! The best part is your own reflection “I find it incredibly difficult and error prone for me to read through code once” and how you use that learning to improve your own skills.
Several studies (google it) shows that not more than 150-200 lines of code should be reviewed per hour, becuse the mind starts to loose foucs.
I only use mentally checklists today but I will definitely give it a try.
Or you could automated the checklist in your build…TFS Build Workflow like a boss with custom activities.
I’m a big fan of incorporating linting and other static analysis into the process, too. A lot of the items that I check at code review time are the ones which are harder to automate. The tooling for static analysis are improving every day, though, so I’m looking forward to the day when I only have to argue about the high level concepts and can let a computer find all the bugs.