Yet another code review checklist

In this short post I would like to share the code review checklist I kept up on my workstation.

Before the review

• Check if the pull request has a good description of the changes. It should at least explain the problem and the solution. The longer, the better. The best if changes contain an ADR or refer to a design document.
• Check the number of added and removed lines. If the pull request is too large, consider suggesting author to split and submit separate PRs. In some cases there may be many changed lines due to new formatting rules or code being moved around.If such changes aren’t done in separate commits, spotting the important changes may be harder. Suggest the author to commit formatting changes in a separate commit.
• Appreciate if a PR can be reviewed commit-by-commit.
• Distant the changes from the author to eliminate biases because it is possible to treat the code by the author. For example; If the author is a senior engineer, one could be biased towards not looking for obvious bugs or be hesitant about suggesting trivial fixes.
• Allocate a larger time slot for reviews that contains changes to unfamiliar projects/modules. Use the extra time to capture the context — the current state of the code and the problem PR aims to solve.

During the review

• Start with a high-level review before going into tiny details. Does the proposed solution make sense? Are there any fundamental flaws with the approach? If so, first leave a comment about this and discuss it with the author before diving into details.
• After agreeing on the high-level approach — check the details, starting with the implementation changes, followed by unit/integration tests.
• Look for ways to suggest comments that simplifies the solution if the code is not easy to read / understand. Code that can’t be understood now will be harder to understand a year later.
• Look for potential edge cases that the changes may fail to handle.
• If the changes occur on modules/classes that has some known tech debt, suggest to improve the code and pay the debt (partially) whenever makes sense. This way tech debt can be paid (or kept in check) gradually.
• If the changes introduce a new 3rd party dependency, check its license. Also check if the dependency has known security vulnerabilities or performance problems.
• Check if any newly added dependencies affect the output (bundle) size.
• Check if changes break backwards compatibility. For example, changes to public APIs should either be versioned, or only have incremental (optional) changes.
• Check if changes may introduce security vulnerabilities such as unsanitized user input or unhandled runtime errors that can leak internal details to client.
• Check the changes for any CPU or IO intensive operations that may degrade performance when the system is under load.
• Check if the changes handle potential exceptions properly.
• Finally, review the code that did not change. For example, the changes may have introduced a new helper function, and there may be other places in the code which it can be reused.
• Don’t review code style / formatting. It is the job of a linter, not a developer.

After the review

• Thank the author for the changes and leave an encouraging comment about the work.
• If there are too many comments, leave out the trivial ones as getting too many comments at once can be discouraging for the author.
• If the same issue occurs in multiple places, use a single comment to refer all of them instead of commenting each instance with “same here”.

That’s it! I intend to keep adding new ones in time. I hope that you find it useful!