Saturday, September 1, 2012

A checklist for code reviews

A few years back, Atul Gavande, an American surgeon, got a lot of press from a study that showed dramatic reductions in deaths and complications from surgery when surgical teams instituted simple checklists. Death rates dropped from 1.5% to 0.8% and serious complications fell from 11% to 7%.

Checklists are not about creativity; they don't help you make dramatic insights. Instead, they're about consistency; they make sure you don't forget the simple, boring stuff. And some of the simple boring stuff is really important.

We in the software industry can use checklists as part of code reviews and inspections. This list covers the major concerns:
  1. (reuse) Is there already code that does something similar? Why is this code not reusing the existing work, perhaps with modifications? 
  2. (correctness) What has been done to verify that this code produces correct results? In particular, what parts of it are verified by automatic tests? 
  3. (clarity) If the purpose of this code or its implementation is obscure, where is it explained?
  4. (documentation) How widely is this code used? Is its documentation appropriate to the breadth of use? 
  5. (data volume) How large a volume of input is this code expected to process? Are the algorithms and data structures appropriate to the task? 
  6. (memory use) Does the code allocate memory? Who takes ownership of it, and how will it eventually be freed? 
  7. (error handling) How does the code report errors or unexpected conditions? Does it propagate error reports upward from code it calls? 
  8. (concurrency) Does this code execute concurrently? What has been done to avoid memory corruption and unnecessary exclusion? 
  9. (execution efficiency) Does this code need to execute quickly? What has been done to ensure it does so? 
  10. (storage efficiency) Does this code need to use storage parsimoniously? What has been done to ensure it does so? 
  11. (security) Does this code access or produce sensitive information? What has been done to keep this information secure? 
  12. (dead code) If this code replaces other code, has the older code been removed?
Checklists are by no means new in the context of software development. Their use is a standard part of formal software inspections as described in Software Inspection by Gilb and Graham. But in my experience, they are rarely (as in, practically never) used in industry. They're a good idea that should be used more widely.

No comments:

Post a Comment