code_review_checklists.md

Based on the article: Using checklists for code review

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.

Obviously, not everything is applicable for every change. If the review request isn't making any changes to UI, then skip the first two checklists entirely. If a change is a bug fix, typically don't review it for architecture and design principles.

Put the big stuff first (e.g. architecture). You don't want to work through a ton of small issues before realizing that everything has to be rewritten.

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.

User interface

• Are screenshots provided for all UI changes?
• Does this change introduce any new concepts or models? Evaluate.
• How much expertise and context will a prospective user need?
• Do we do anything similar elsewhere? Can we build on existing knowledge or habits?
• Do we do anything similar but just different enough to cause problems with old habits?
• Are there any new multi-step workflows? How long and complex are they?
• If users make mistakes or errors, what are the failure conditions? Can mistakes be undone?
• How is the copy? Are sentences well-formed and clear? Any spelling errors or typos?
• Can the UI layout be localized into different language editions without any changes to the source code?

Visual design (native desktop app)

• Are controls laid out in a way which makes sense given visual scan order?
• Check for alignment
• Check for layout, spacing, and padding
• Check for visual consistency

Correctness (C/C++)

• Correctness of algorithms
• Loop iteration and off-by-one
• Memory access errors
• Memory leaks
  e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.
• Incorrect behaviour
• Switch/break fallthrough
• Exception safety

Security (C/C++)

• Array bounds on buffers
• Are files created and read/written? Check permissions, races
• All user input sanitized or validated?
• Proper, bounded use of str*, printf, f* functions?
• Validate remote host cert validity & identity
• Crypto use