Using checklists for code review

It sounds minor, but I think it's pretty important: The points on his checklist aren't clear yes/no questions, they're a bunch of things you should remember to consider.

Specifically, how do you check "Correctness of algorithms"? What is the criteria to pass that? "Well, I think it's right", of course you do, otherwise you wouldn't have committed it. Formal proof? Unit tests?

Items phrased like this are ripe to become a "looks fine, check" style process-theatre that doesn't actually add anything.

Credit where credit it due, "Are controls laid out in a way that makes sense given visual scan order?" is a bloody good one.

Checklists can be very helpful to make sure you remember to consider everything in the right order. I have a phone screen checklist that includes "introduce yourself" because sometimes I forget and that's embarrassing. At a previous job, we had a "stakeholders that might be affected" checklist used in feature planning (we had gotten into a bad habit of building features that affected the performance profile of the app, causing ops to get paged on the weekend because they hand't been told to expect different behavior, also: removing an arcane feature that we had wrongly assumes nobody used).

One thing to remember when comparing with flight or medical checklists: These people deal in repeating almost the same thing as consistently as possible. Software engineering is rarely like that.

There's a nice list of checklists at

http://www.projectcheck.org/checklists.html

including a "checklist for checklists" (!)

http://www.projectcheck.org/checklist-for-checklists.html

Great article on the use of checklists in hospitals, by Atul Gawande:

http://www.newyorker.com/reporting/2007/12/10/071210fa_fact_...

As much as people feel negatively about Java, it really has fantastic static code analysis tools. PMD, FindBugs, CheckStyle are all very helpful when properly configured. They're basically on the checklist level.

I think checklist level thinking for code reviews is bad though. It ends up being a pretty soul destroying experience when someone tells you that you need to validate arguments to all methods as not null (for the 100 you wrote), or that you should take the data in your test cases and put it in a text file. Or that you've re-used the same string in several places and should make it constant. Robots can tell me that, and I can get their feedback while I'm developing or choose to ignore it. People are invaluable for helping you step back and see how to re-organize your code, introduce new abstractions, re-interpret requirements, and see actual bugs that no QA person or tool would ever discover (likely affecting some unlucky user who would hit an extremely rare bug that couldn't be reproduced easily). Those types of reviews are invaluable. Put down the checklist and automate it instead.

Some of these can be caught automatically, and the more automation (taking the human out of the loop), the better, because humans can gloss over a checklist or accidentally skip something. My current project uses a combination of Gerrit, Jenkins and Sonar, along with unit/regression tests, so that every single change is checked automatically to make sure it builds and passes the tests, along with requiring at least one human review and okay it. Combine this with code checking tools (vera++, rats, cppcheck, cpplint, flawfinder, pscan, valgrind, fuzz, along with just about every warning available on two compilers), and we hardly ever have to do anything but look at code in Gerrit and approve it.

We've tried using checklists a bit for code review, and hit the problem that our checklists would be pretty much guaranteed to have at least one item that was arguably inapplicable to the changeset. So you'd have some changeset that was very small, or not contain UI elements, or did contain UI elements, or was related to build tools, and so on.

And so we'd get into a situation where we'd have to decide whether a particular item was even applicable or not, which somewhat defeats the point of checklists, since you're supposed to be able to decide immediately whether an item is satisfied or not.

I like it. A checklist is a very lightweight and efficient way for a team to codify a list of objective things that they feel must be done and checked for every pull-request. Once that's done, it should be considered good to merge regardless of peoples subjective opinions about the code. This avoid drawn out and opinionated debates about the code dragging down productivity. That isn't to say a bit of debate isn't a good thing, it just need not be the main thing.

I have yet to see a code review checklist that I find useful. There are just too many things that can go wrong with software, so the list ends up either too long to be practical, or too generic to be helpful.

What I do find useful is to have a good variety of skills on the review team. Different people tend to check for different things. Some are just naturally good at spotting, say, concurrency problems, others have a pet peeve with names that aren't clear, etc. But someone who doesn't have an eye for concurrency problems won't magically start noticing them in a pile of code because there's a box saying "check for race conditions" somewhere on the list.

I just think its rather silly to do code reviews this way unless you're writing software to power a space shuttle (ie stuff that should never fail/break), It would simply take too long.

Couldn't a checklist also be the tests written for the feature? This ensures that future revisions don't muck with the already working system.

Checklists are OK, but beware the checklist. They tend to cause complacency. "It wasn't in the checklist!"