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.
- 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?
- Minimal cognitive overhead?
i.e. How many logical connections or jumps does your brain have to make in order to understand or contextualize the thing you’re looking at? - Does the user get appropriate (real-time) feedback?
- Usable by the young, old, and drunk?
- Ask users/customers to repeat what your product is for and how to you use it, after they've tried it.
- 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
- Does the architecture make sense?
- Is the most appropriate algorithm used for the job?
- Does a better and proven 3rd party solution already exist?
- 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
- 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
- Unit tests written?