Ultimate Code Review Checklist

Posted by

I’ve been doing code review in one form or another all my career. For the most part, I’ve been haphazardly looking at code, trying to find the obvious improvements or defects. It’s much more difficult to find a missing test case, an exploitable security hole, or a race condition. Enter the checklist! Long used by pilots to prepare an airplane for the every phase of a flight, it serves us in much the same way for code review.

Why Use A Checklist?

It helps us make sure we haven’t forgotten to look for something and points us in the right direction when we’re feeling a bit lost looking at some new code. Because the list of things we want to check for is non-trivial, the checklist makes sure that each item has been addressed.

What makes this checklist different from the others?

Other code review checklists I’ve seen are either too long or too high-level, making them difficult to use in traditional checklist fashion. This list, while shorter than most, focuses on the highest importance issues that should be addressed. It goes without saying that you’ll want to refine this list to fit your language-specific needs.

Without further ado, let’s get to the list!

The List

General

  • “Getting Started” documentation is complete and up to date with the latest changes
    • A short description of what the code does and how it does it
    • Contact information for the owners of the code
    • How to run the unit and integration tests
  • Is the naming of variables, classes, and namespaces clear?
  • Is the logging sufficient for production debugging purposes?
  • Can any of the code be replaced with library functions?

Design

  • Don’t Repeat Yourself: Anything more than 1 line of repetition can generally benefit from a refactoring.
  • Single Responsibility Principle:  Ensure it is respected in any new method or class.
    • Are the classes sufficiently small?
    • Are there a lot of dependencies needed for the class to do its work?
    • A single class does stuff, or knows other classes that do stuff, but not both.
  • Is the code testable?
    • Can a low-impact change be made to improve testability?
    • Are dependencies easily stubbed out?
    • Should some code be moved to its own class to make testing easier?
  • Is the new code respecting the architectural patterns laid out by the team (n-Tier, Ports and Adapters, the list goes on…)
  • Are some things executed in a more complex fashion than they need to be?
  • Do any of the methods have too many arguments?
  • Clear Conditional Logic
    • Any sufficiently difficult conditional statement should be encapsulated in a method with a name that clearly identifies what we’re checking
    • A positive conditional is easier to read than a negative conditional
  • Are the asynchronous components implemented correctly? (Promises, async/await, ReactiveX…)

Readability

  • Is complex logic self documenting via method names?
  • Is the commenting sufficient without being excessive?
  • Is there any commented out code that can be removed?
  • Are there any TODOs that haven’t been addressed or don’t have an associated issue number?
  • Are there any typos in the code or comments?
  • Are constants used in place of magic numbers?

Tests

  • Are all the successful test cases covered?
  • Are all the error test cases covered?
  • Are the tests easy to understand?
  • Are the tests reliable? Any test that are flakey need to be fixed or removed
  • Are exceptions handled properly in the larger context of the application?

Security

  • Are all inputs checked for the correct type, length, format and range?
  • Are invalid parameter values handled properly?
    • Exceptions for which the user can change something get returned as a 400-level errors
    • Exceptions for which the user can’t do anything about are returned as 500-level errors
  • For any error thrown by a 3rd party library, are they being caught and handled accordingly?
  • Are any internal implementation details being leaked through the error and exceptions messages?
  • Are credentials or URLs hard-coded in the source code?
    • They should be moved to environment variables
    • Make sure that you erase them from the entire history of the source code!
  • Credentials are never logged, or are obfuscated when written to logs

How To Use This List Effectively?

Generally, you use a checklist by carefully going over every item, ensuring that every item is completed.  Depending on your code review tool, this approach may not work too well for you. Instead, do a first pass of the code to get a grasp of the changes, then a second pass looking for any mistakes you see, cross-referencing with the checklist to make sure you didn’t miss anything. Eventually, the list will become second nature and you may not even need it.

So there we have it. The first revision of the Ultimate Code Review Checklist. If there are any important items that I’ve forgotten, please drop me a comment below!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s