Code Review Checklist: 62 Things to Look Out For in Code Reviews
Tags: codereview, programming
Published: 2022-02-23 14:35:27

---
canonical_url: https://davidadamojr.com/code-review-checklist/
description: Do you use a code review checklist to encourage thorough code reviews? Here are 62 things to look out for in code reviews.
---

<!-- wp:paragraph -->
<p>What do you look out for in your code reviews? Is the quality of your code reviews consistent? Do you use a code review checklist?</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>There are several reasons why the quality and thoroughness of code reviews may vary significantly across pull requests. Some reasons for inconsistency in code reviews include:</p>
<!-- /wp:paragraph -->

<!-- wp:list -->
<ul><li>The skill and competence level of the code reviewer.</li><li>How much the code reviewer knows about the specific issue(s) addressed in the code change.</li><li>The time of day when the code review was performed.</li><li>The specific programming issues that have been on the reviewer's mind lately.</li><li>The reviewer's perception of the code contributor's competence.</li><li>The programming languages and technologies used in the code change.&nbsp;</li></ul>
<!-- /wp:list -->

<!-- wp:paragraph -->
<p>A code review checklist is a useful way to improve the quality and consistency of code reviews. Consistent use of checklists decreases the likelihood that a reviewer's focus is skewed toward specific areas of interest or convenience at the expense of other important areas.&nbsp;</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p><a href="https://www.michaelagreiler.com/" title="Dr. Michaela Greiler" target="_blank" rel="noreferrer noopener">Dr. Michaela Greiler</a> has done some remarkable work on helping software engineers improve their code reviews. This post presents a checklist, based on Dr. Greiler's work, with 62 things to look out for in code reviews. The questions in this checklist are structured such that each question can be answered with either a YES, NO, or Not Applicable (N/A). In theory, the likelihood that a code change is of acceptable quality is inversely proportional to the number of “NO” answers to the questions in the checklist.&nbsp;Many of the questions in this checklist are subjective and almost completely at the discretion of the code reviewer.</p>
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>Code Review Checklist</h2>
<!-- /wp:heading -->

<!-- wp:heading {"level":3} -->
<h3>PR Etiquette</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Does this code change address a <a target="_blank" href="https://fagnerbrack.com/one-pull-request-one-concern-e84a27dfe9f1" rel="noreferrer noopener">single high-level concern</a>?</li><li>Are the commit messages <a target="_blank" href="https://blog.oddbit.com/post/2019-06-14-git-etiquette-commit-messages/" rel="noreferrer noopener">clear and well-written</a>?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Implementation</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Does this code change do what it is supposed to do?</li><li>Is there a <a target="_blank" href="https://www.infoq.com/articles/simplicity-manifesto-development/" rel="noreferrer noopener">simpler</a> alternative to this code change?</li><li>Are all the new <a target="_blank" href="http://tutorials.jenkov.com/ood/understanding-dependencies.html" rel="noreferrer noopener">compile-time and run-time dependencies</a> in this code change necessary and appropriate?</li><li>Does this code change use appropriate frameworks, APIs, libraries, and services instead of custom code whenever possible?</li><li>Is this code change at the right <a target="_blank" href="https://8thlight.com/blog/javier-garc%C3%ADa/2019/06/11/refactoring-levels-of-abstraction.html" rel="noreferrer noopener">level of abstraction</a>?</li><li>Is the code <a target="_blank" href="http://singlepageappbook.com/maintainability1.html" rel="noreferrer noopener">modular</a> enough?</li><li>Does this code change use existing code whenever possible instead of introducing new code?</li><li>If this code change does not use similar existing functionality in the codebase, is there a good reason why? </li><li>Does this code change use best practices, <a target="_blank" href="https://refactoring.guru/design-patterns" rel="noreferrer noopener">design patterns</a> or language-specific patterns in ways that improve code maintainability, readability, performance, and security?</li><li>Does this code appropriately follow <a target="_blank" href="http://www.cs.utsa.edu/~cs3443/notes/designPrinciples/designPrinciples.html" rel="noreferrer noopener">Object-Oriented Analysis and Design principles</a>?</li><li>Do all classes, methods, and functions have a <a target="_blank" href="https://martinfowler.com/bliki/FunctionLength.html" rel="noreferrer noopener">reasonable number of lines</a>?</li><li>Does this code change add clear value and avoid <a target="_blank" href="https://en.wikipedia.org/wiki/Feature_creep" rel="noreferrer noopener">feature creep</a>?</li><li>Does this code change adhere to the <a target="_blank" href="https://programming-idioms.org/all-idioms" rel="noreferrer noopener">idioms and code patterns</a> of the programming language?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Logic Errors and Bugs</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Does this code change behave as intended for all use cases you can think of?</li><li>Does this code change properly handle unconventional inputs and external events without breaking?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Error Handling and Logging</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Does this code change <a target="_blank" href="https://owasp.org/www-community/Improper_Error_Handling" rel="noreferrer noopener">handle errors correctly</a>?</li><li>Are error messages <a target="_blank" href="https://xd.adobe.com/ideas/process/information-architecture/error-message-design-ux/" rel="noreferrer noopener">descriptive and user-friendly</a>?</li><li>Does this code change produce <a target="_blank" href="https://www.dataset.com/blog/the-10-commandments-of-logging/" rel="noreferrer noopener">proper logs and debugging information</a>?</li><li>Are <a target="_blank" href="https://www.orpiske.net/2011/10/writing-good-log-messages/" rel="noreferrer noopener">log messages</a> written in a way that enables easy debugging?</li><li>Does this code change ensure that logs do not <a target="_blank" href="https://cwe.mitre.org/data/definitions/532.html" rel="noreferrer noopener">expose too much technical information</a> that could lead to a security issue?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Usability and Accessibility</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Is the proposed solution well designed from a <a target="_blank" href="https://www.nngroup.com/articles/usability-101-introduction-to-usability/" rel="noreferrer noopener">usability</a> perspective?</li><li>Does the proposed solution adequately address <a target="_blank" href="https://www.washington.edu/doit/designing-software-accessible-individuals-disabilities" rel="noreferrer noopener">accessibility concerns</a>?</li><li>Are the APIs defined in this code change <a target="_blank" href="https://idratherbewriting.com/learnapidoc/" rel="noreferrer noopener">well documented</a>?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Ethics and Morality</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Does this code change use data in a way that minimizes or eliminates privacy concerns?</li><li>If this code change adds or alters ways in which people interact with each other, are appropriate measures in place to prevent/limit/report harassment or abuse?</li><li>Does this code change adequately avoid exploitation of behavioral patterns or human weaknesses?</li><li>Does this code change adequately ensure that it does not cause mental or physical harm to users?</li><li>Does this code change adequately ensure that it does not exclude certain groups of people or users?</li><li>Does this code change adequately avoid introducing any algorithm, AI or machine learning <a target="_blank" href="https://en.wikipedia.org/wiki/Algorithmic_bias" rel="noreferrer noopener">bias</a>?</li><li>Does this code change adequately avoid introducing any gender/racial/political/religious/ableist <a target="_blank" href="https://en.wikipedia.org/wiki/Algorithmic_bias" rel="noreferrer noopener">bias</a>?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Testing and Testability</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Is the code well designed from a <a target="_blank" href="https://www.infoq.com/articles/Testability/" rel="noreferrer noopener">testability</a> perspective?</li><li>Does the code change have <a target="_blank" href="https://personal.utdallas.edu/~ewong/SE6367/03-Lecture/15-Test-Adequacy-one-slide.pdf" rel="noreferrer noopener">enough</a> automated unit, integration, and system tests? If not, do the existing tests adequately cover the code change?</li><li>Do the test cases in the code change cover all important inputs or edge cases you can think of?</li><li>Are the tests <a target="_blank" href="http://agileinaflash.blogspot.com/2009/02/first.html" rel="noreferrer noopener">well isolated and independent</a> from one another?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Dependencies</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Have necessary updates been made to other artifacts related to this code change (e.g. documentation, configuration, README files, etc.)?</li><li>Does this code change adequately consider and address any undesirable effects on other parts of the system?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Security and Data Privacy</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Have all new dependencies been <a target="_blank" href="https://docs.npmjs.com/auditing-package-dependencies-for-security-vulnerabilities" rel="noreferrer noopener">audited for security vulnerabilities</a>?</li><li>Does this code change <a target="_blank" href="https://owasp.org/www-project-mobile-top-10/2014-risks/m5-poor-authorization-and-authentication" rel="noreferrer noopener">appropriately handle authentication and authorization</a>?</li><li>Is sensitive information like user data and credit card information securely handled and stored?</li><li>Does this code change use appropriate encryption schemes when necessary?</li><li>Does this code change properly hide secret information like keys, usernames, and passwords?</li><li>Does this code change properly address security vulnerabilities such as cross-site scripting and SQL injection? Does this code change properly sanitize and validate user input?</li><li>Does this code change properly validate data retrieved from external APIs or libraries?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Performance, Reliability, and Scalability</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Is this code change unlikely to have a significant negative impact on system performance?</li><li>Does this code change include all the useful performance optimizations that you can think of?</li><li>Does this code change have instrumentation for metrics reporting and failure alerts?</li><li>Does this code change use appropriate built-in functions wherever possible?</li><li>Has all debugging code been removed from this code change?</li><li>Are all performance optimizations in this code change necessary?</li><li>Does this code change use some form of <a target="_blank" href="https://en.wikipedia.org/wiki/Cache_(computing)" rel="noreferrer noopener">caching</a> whenever appropriate?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Readability</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>Is the code easy to understand?</li><li>Are methods and functions <a target="_blank" href="https://martinfowler.com/bliki/FunctionLength.html" rel="noreferrer noopener">small enough</a> to be easily readable?</li><li>Are function, method and variable names <a target="_blank" href="https://wiki.c2.com/?MeaningfulName" rel="noreferrer noopener">meaningful</a>?</li><li>Are modules located in appropriate and <a target="_blank" href="https://wiki.c2.com/?MeaningfulName" rel="noreferrer noopener">meaningfully named</a> files, folders, and packages?</li><li>Is <a target="_blank" href="https://en.wikipedia.org/wiki/Control_flow" rel="noreferrer noopener">control flow</a> in modules, functions, and methods intuitive?</li><li>Is the data flow in modules, functions, and methods intuitive?</li><li>Are all comments in this code change necessary and meaningful?</li><li>Do the comments in this code change describe why and not what?</li><li>Does this project use a linter?</li></ul>
<!-- /wp:list -->

<!-- wp:heading {"level":3} -->
<h3>Secondary Review</h3>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li>If this code change impacts different teams, has it been reviewed by people from the affected teams?</li></ul>
<!-- /wp:list -->

<!-- wp:paragraph -->
<p>This code review checklist is also available as a <a href="https://docs.google.com/spreadsheets/d/11Vk6Wi2nOLl5bVlan5w8iGDBwBXekVit5q1yDAD_L7M/edit?usp=sharing" target="_blank" rel="noreferrer noopener" title="spreadsheet">spreadsheet</a> to edit and improve as you please.&nbsp;</p>
<!-- /wp:paragraph -->