Skip to content

Instantly share code, notes, and snippets.

@dideler
Last active July 23, 2024 05:43
Show Gist options
  • Save dideler/5912225 to your computer and use it in GitHub Desktop.
Save dideler/5912225 to your computer and use it in GitHub Desktop.
Code review checklists. Leave your suggestions in a comment below!

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?
  • 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.

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

Logic & Correctness (C/C++)

  • 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

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

Tests

  • Unit tests written?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment