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?
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:
- The skill and competence level of the code reviewer.
- How much the code reviewer knows about the specific issue(s) addressed in the code change.
- The time of day when the code review was performed.
- The specific programming issues that have been on the reviewer's mind lately.
- The reviewer's perception of the code contributor's competence.
- The programming languages and technologies used in the code change.
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.
Dr. Michaela Greiler 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. Many of the questions in this checklist are subjective and almost completely at the discretion of the code reviewer.
Code Review Checklist
PR Etiquette
- Does this code change address a single high-level concern?
- Are the commit messages clear and well-written?
Implementation
- Does this code change do what it is supposed to do?
- Is there a simpler alternative to this code change?
- Are all the new compile-time and run-time dependencies in this code change necessary and appropriate?
- Does this code change use appropriate frameworks, APIs, libraries, and services instead of custom code whenever possible?
- Is this code change at the right level of abstraction?
- Is the code modular enough?
- Does this code change use existing code whenever possible instead of introducing new code?
- If this code change does not use similar existing functionality in the codebase, is there a good reason why?
- Does this code change use best practices, design patterns or language-specific patterns in ways that improve code maintainability, readability, performance, and security?
- Does this code appropriately follow Object-Oriented Analysis and Design principles?
- Do all classes, methods, and functions have a reasonable number of lines?
- Does this code change add clear value and avoid feature creep?
- Does this code change adhere to the idioms and code patterns of the programming language?
Logic Errors and Bugs
- Does this code change behave as intended for all use cases you can think of?
- Does this code change properly handle unconventional inputs and external events without breaking?
Error Handling and Logging
- Does this code change handle errors correctly?
- Are error messages descriptive and user-friendly?
- Does this code change produce proper logs and debugging information?
- Are log messages written in a way that enables easy debugging?
- Does this code change ensure that logs do not expose too much technical information that could lead to a security issue?
Usability and Accessibility
- Is the proposed solution well designed from a usability perspective?
- Does the proposed solution adequately address accessibility concerns?
- Are the APIs defined in this code change well documented?
Ethics and Morality
- Does this code change use data in a way that minimizes or eliminates privacy concerns?
- 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?
- Does this code change adequately avoid exploitation of behavioral patterns or human weaknesses?
- Does this code change adequately ensure that it does not cause mental or physical harm to users?
- Does this code change adequately ensure that it does not exclude certain groups of people or users?
- Does this code change adequately avoid introducing any algorithm, AI or machine learning bias?
- Does this code change adequately avoid introducing any gender/racial/political/religious/ableist bias?
Testing and Testability
- Is the code well designed from a testability perspective?
- Does the code change have enough automated unit, integration, and system tests? If not, do the existing tests adequately cover the code change?
- Do the test cases in the code change cover all important inputs or edge cases you can think of?
- Are the tests well isolated and independent from one another?
Dependencies
- Have necessary updates been made to other artifacts related to this code change (e.g. documentation, configuration, README files, etc.)?
- Does this code change adequately consider and address any undesirable effects on other parts of the system?
Security and Data Privacy
- Have all new dependencies been audited for security vulnerabilities?
- Does this code change appropriately handle authentication and authorization?
- Is sensitive information like user data and credit card information securely handled and stored?
- Does this code change use appropriate encryption schemes when necessary?
- Does this code change properly hide secret information like keys, usernames, and passwords?
- 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?
- Does this code change properly validate data retrieved from external APIs or libraries?
Performance, Reliability, and Scalability
- Is this code change unlikely to have a significant negative impact on system performance?
- Does this code change include all the useful performance optimizations that you can think of?
- Does this code change have instrumentation for metrics reporting and failure alerts?
- Does this code change use appropriate built-in functions wherever possible?
- Has all debugging code been removed from this code change?
- Are all performance optimizations in this code change necessary?
- Does this code change use some form of caching whenever appropriate?
Readability
- Is the code easy to understand?
- Are methods and functions small enough to be easily readable?
- Are function, method, and variable names meaningful?
- Are modules located in appropriate and meaningfully named files, folders, and packages?
- Is control flow in modules, functions, and methods intuitive?
- Is the data flow in modules, functions, and methods intuitive?
- Are all comments in this code change necessary and meaningful?
- Do the comments in this code change describe why and not what?
- Does this project use a linter?
Secondary Review
- If this code change impacts different teams, has it been reviewed by people from the affected teams?
This code review checklist is also available as a spreadsheet to edit and improve as you please.