Reviewing Code More Effectively

I believe that it’s important not only to do code reviews when developing software, but also to really understand why we do code reviews, how to make code reviews useful and effective, and what to look for in a code review.

Why Do We Do Code Reviews?

Fundamentally, a code review serves to answer three questions, loosely in descending order of importance:

1. Is a proposed change to the code a net improvement to the codebase as a whole?
2. How might the proposed change be improved upon?
3. What can the reviewer(s) (more broadly, the team) learn from the proposed change?

I want to especially draw your attention to the nuances of that first question. Code is never perfected, and expecting perfection in a code review can be counterproductive and frustrating to the contributor. The goal of a code review is not perfect code, but better code—the code can always be further improved in the future if it proves necessary to do so. Keeping that in mind can make for more efficient reviewing.

How Do We Make Code Reviews Effective?

As a maintainer of a software project, you should prioritize setting contributors up for successful code reviews. The details may vary depending on the size of the project and its pool of potential contributors, but ideas that generally apply include:

• Labeling and categorizing open issues (bugs, feature requests, etc.) helpfully, especially in terms of their complexity or any specialized expertise that might be needed to tackle a specific issue. (Many open-source projects use a label such as help wanted or good first issue as a way to highlight issues that would be well-suited for a new contributor to take on, as one example.)
• Documenting expectations for code contributions clearly, through tools such as GitHub’s “pull request templates” and CONTRIBUTING.md, as well as by providing well-written developer documentation in general.
• Automating any part of the project’s requirements that can be effectively automated, including unit and regression tests, but also extending to tools such as linters, spell-checkers, and code autoformatters.

As a contributor to a software project, key points to keep in mind include:

• Solve one problem at a time—the smaller and more self-contained a code review is, the more easily and effectively it can be reviewed.
• Provide a thorough explanation of the reasons behind the proposed code change—both as code comments and as the “description” or “summary” attached to the code review request, which can and should include screenshots, example payloads, and so forth.
• Provide testing to demonstrate that the change does what it sets out to do. (Ideally automated, but even a well-documented manual test is far better than nothing!)
• Approach the code review as a learning experience, and take feedback with an open mind.

As a reviewer of a code review, you should:

• Approach the code review as both a teaching experience (sharing your hard-won expertise with the current code) but also as a learning experience.
• Provide feedback politely and without ego (no matter how tempting it may be to regard your own existing code as impossible to improve upon!).
• Link to relevant documentation and best practices to clarify and support any feedback you provide.

What Should We Look For in a Code Review?

I like to think of different approaches to a code review as a series of distinct frames of mind, or “hats” that I might “wear”. You can also think of “wearing a hat” as assuming a different persona as a reviewer, then focusing on areas that are important to that persona. In practice, there are no firm dividing lines between these, and I’ll often “wear” many “hats” at once as I’m doing a code review, but it can be a useful checklist to keep in mind for thoroughness.

The hats that I’ll discuss here are:

• Beginner
• User
• Developer
• Tester
• Attacker
• Maintainer

Hat of the Beginner

This involves an approach often labeled as “beginner’s mind” in other contexts. Fundamentally, the goal is to approach the code without preconceptions or assumptions, being unafraid to ask questions and seek clarification. The key skill here is curiosity. For example, you might ask:

• Is the code understandable and well-documented?
• Does this code actually do what the function name, comments, docstring, and so forth imply that it should do?
• What might happen if this conditional logic check evaluates as False rather than True?
• What might happen if a user provides “weird” or “invalid” inputs?
• All in all, does the code change “make sense”?

Hat of the User

When wearing this hat, you focus on the experience of the user of this software. This could be a human user, but could also be another piece of software that interacts with this project via an API of some sort. Example questions to ask as a user might include:

• Is the UI or API sensible, predictable, usable?
• Is the operation of the software appropriately observable (via logging, metrics, telemetry, and so forth)?
• Does the proposed code change introduce breaking changes to the existing experience of the user?
• Does the proposed code change follow the principle of least surprise?
• Is the code change clearly and correctly documented at all appropriate levels?

Hat of the Developer

This hat is what many of us may think of first when we think of approaches to code review, and it absolutely is a useful and important one at that. This approach focuses heavily on the details of the code and implementation, asking questions like:

• Can this code be understood and maintained by developers other than the original author?
• Is it well-designed, useful, reusable, and appropriately abstracted and structured?
• Does it avoid unnecessary complexity?
• Does it avoid presenting multiple ways to achieve the same end result?
• Is it DRY?
• Does it include changes that aren’t actually needed at this time (YAGNI)?
• Does it match the idioms and style of the existing code?
• Is there a standard or “off the shelf” solution that could be used to solve this particular problem instead of writing new code?

Hat of the Tester

This hat is my personal favorite, as I started my career as a software tester. The tester’s goal is to think of what might go wrong, as well as what needs to go right. You might ask:

• Does this change meet all appropriate requirements (explicitly stated as well as implicit ones)?
• Is the logic correct, and is it testable to demonstrate correctness?
• Are the provided tests correct, useful, and thorough?
• Does the code expect (and appropriately handle) unexpected inputs, events, and data?

… there are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies, and the other way is to make it so complicated that there are no obvious deficiencies. – Sir Tony Hoare

Hat of the Attacker

This is another fun hat (or maybe I just like finding problems?). The attacker’s hat is closely related to the tester’s hat, but takes on a more devious frame of mind. Here you should ask questions like:

• Does the code show adequate performance under usual (and unusual) conditions?
• Can I make the code crash or throw an exception?
• Can I make the code access something I shouldn’t be able to?
• Can I make the software break, corrupt or delete data, or otherwise do the unexpected?

Hat of the Maintainer

This is the big-picture counterpart to the detail-focused developer’s hat. Here you should be asking questions like:

• If certain issues or nitpicks keep coming up in review after review, is there something we should automate in the future?
• Does the code change fit into the broader context of the project?
• Are any new dependencies being added to the project? And if so, are they acceptable (actively maintained, appropriately licensed, and so forth)?

References and Further Reading

Many others have also written about the art and science of effective code reviews. Here are a few that I found particularly useful:

• Author’s Guide to Getting Through Code Review
• How to Do a Code Review
• The Code Review Pyramid
• Best Practices for Maintainers
• How to Make Good Code Reviews Better
• Giving Better Code Reviews