What Belongs in an Effective Code Review Checklist?

What Belongs in an Effective Code Review Checklist?

images/nesa-by-makers.jpg
Photo by NESA by Makers on Unsplash

As engineering teams become more established, the need to formalize a code review process becomes more important. At PullRequest, we’ve observed time and time again one of the most frequented formalization practices is to compose a code review checklist that can be applied to every pull request that’s opened by the team.

However, for checklists to be effective, teams need to use them consistently and comprehensively with each code review. That imposition can make checklists controversial, especially within engineering teams that resist process.

So, what separates the good code review checklists from ineffective ones?

  • A good checklist helps reviewers move faster and review in a more reliable and consistent manner by reducing the amount of information that the reviewer needs to remember and carry in their head. It focuses on the highest-priority issues and doesn’t overwhelm the reviewer with too much to do.

  • On the other hand, a bad checklist encourages nitpicking, ignores the importance of velocity, and unintentionally hinders progress. If the checklist is overbearing, redundant with other processes, or not consistently applicable, it can become useless—something code authors and reviewers will tend to completely ignore without guilt or hesitation.

We looked at the code review checklists of four software companies—Google, Yelp, Squarespace, and Fog Creek—to see how top software teams design their code review checklists.

Since every development team is different, reusing another company’s checklist verbatim is usually a recipe for inefficiency. Use these checklists as starting points to reflect on what your engineering team needs. We’ve broken it down into commonly recurring checklist sections with checklist item examples from those software businesses to help you get started.

Code Design

Code design is where automation is least able to help. Linting engines can catch small-scale style issues, and automated tests ensure that the code works according to test spec, but only a human reviewer can evaluate how well the code was designed. An expert reviewer from PullRequest, for example, can catch important design issues that automated services can’t.

The engineers at Google consider design fundamental to a code review checklist: “The most important thing to cover in a review is the overall design.” The Google team suggests this section of your code review checklist includes questions like:

Do the interactions of various pieces of code in the CL (change list) make sense?
Does this change belong in your codebase, or in a library?
Does it integrate well with the rest of your system?
Is now a good time to add this functionality?

When it comes to design, per Google, “there is no such thing as ‘perfect’ code—there is only better code.”

Test coverage

At Yelp, review for code correctness—“that the code is bug-free, solves the intended problem and handles any edge cases appropriately”—is coupled with a thorough review of the test spec to ensure that a great review done by a human will live on in their automated testing.

Yelp reviewers should “spend time reviewing the testing strategy to ensure that all code is well tested . . . includ[ing] unit tests, integration tests, regression tests, and so on.” The Yelp code review checklist includes:

Verify that the code is tested well
Confirm adequate test coverage
Check tests having the right dependencies and are testing the right things

To make it easier to pass this check during code review, Yelp engineers advise that PRs should be kept as short as possible. Per Yelp, “smaller code changes are also easier to test and verify as stable.”

Readability

While code is meant to be executed by machines, it has to be read and understood by the humans who maintain it and build on top of it. A readability review can slow down the speed of new software releases in the short-term. But by decreasing the amount of developer time spent confused, puzzling over what another dev or that dev themselves wrote six months ago, it can increase iteration speed in the medium to long-term.

At Squarespace, “[t]he goal of the readability pass is to make sure that the person who reads the code in six months will be able to quickly build a coherent mental model of the code.” To ensure this during code review, the Squarespace engineering team includes checklist items like:

Is the change reasonably understandable by humans with little or no prior experience in the code base? Pretend you know the language, but not the code base. Would everything read easily to you? If not, why?
Are any esoteric language features being used? Esoteric language features, while occasionally useful, often hurt readability, even among language experts. If you see esoteric language features being used, ask if a simpler construct would work. If not, make sure that the feature is commented or otherwise documented to decrease cognitive overhead.

Note that Google requires readability review for every CL (change list), while at Squarespace, engineers have discretion over whether to execute the readability review checklist. That flexibility is built into Squarespace’s checklist where they instruct reviewers to be flexible to the “‘shape’ of a particular PR (pull request)” and to “be as thorough as the PR needs.”

Documentation

If documentation is an important part of your engineering culture, including it in your code review checklist encourages devs to document their code in chunks, as it’s written. Documentation written at the time code is merged will tend to be higher quality and more accurate than if a developer was to revisit it days, weeks or months later.

The engineering team at Fog Creek—the company behind Stack Overflow and Trello—now called Glitch, asks these questions:

Do comments exist and describe the intent of the code?
Are all functions commented?
Is any unusual behavior or edge-case handling described?
Is the use and function of third-party libraries documented?
Are data structures and units of measurement explained?
Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like “TODO”?

To get everyone on your team on board with the checklist and what’s expected when it comes to documentation, the Fog Creek team recommends “shar[ing] the list with your team and get[ing] their agreement on its contents.” That’ll “drive up coding standards and avoid inconsistent code review quality.”

Security

Although not everyone is a security expert, effective code review checklists ask reviewers to look for common, recognizable security flaws. If a PR modifies critical parts of the code base, such as payments, user permissions, or user authentication, your team can require multiple reviewers to check it before approval.

The engineering team at Squarespace suggests reviewers familiarize themselves with common threat vectors and interrogate every code change for potential security vulnerabilities:

Is this change secure? Most experienced programmers should know at least a few patterns of application programming that make your code less safe. If you write database code, you should know what a SQL injection vulnerability looks like. If you write frontend code, you should know what an XSS vulnerability looks like. If you’re ever unsure, or if you’re making potentially risky changes, tag someone else from your team on the review.

When in doubt, loop in your senior engineer or security team (if you have one). Code review is your first line of defense against threat vectors.

Conclusion

Prioritizing checklist fundamentals is a valuable, necessary part of good code review practices at organizations of all sizes. By guiding reviewers through code design, test coverage, readability, documentation, and security questions, an effective checklist enables comprehensiveness and helps reviewers move faster.

With a code review checklist, reviewers don’t risk missing the fundamentals and can prioritize their review based on the business problem each code component purports to solve.

If you want to know more about these companies’ code review checklists, you can read:

With a good checklist in your code review arsenal, you can have better reviews and, ultimately, create better code.


About PullRequest

HackerOne PullRequest is a platform for code review, built for teams of all sizes. We have a network of expert engineers enhanced by AI, to help you ship secure code, faster.

Learn more about PullRequest

Lyal Avery headshot
by Lyal Avery

November 11, 2019