Best Practices When Doing Code Reviews — Part 1

Developing software is a team effort that requires the cooperation of multiple people. One example of that cooperation is that of code reviews, a process where code written by a programmer is reviewed by at least one other person. Although there’s a consensus that code reviews are a healthy practice, there isn’t clear guidance about how to do effective code reviews. This series of blog posts will share some general recommendations that we follow at PSPDFKit.

The Main Goal of a Code Review

Before going into the details, it’s important to know what the main goal of a code review is, which is to answer the question “Does this code, when integrated, make the codebase better?” Note that we don’t necessarily need to answer the question “Is this code the best possible code?” The obvious reason is that there’s no single definition of “the best possible code.” Engineering is all about tradeoffs, and choosing a solution that’s already implemented over the other possible ones is one of those tradeoffs.

What to Look For in a Code Review

At PSPDFKit, every team is free to organize the way they want, but every team does code reviews. Reviewers try to focus on the following things, sorted by order of importance:

• Is this new code necessary? What does it try to solve?

• Are there any tests? If so, are they good tests?

• Is the design good? Does it follow good design practices without overengineering?

• Is the code simple and easy to follow?

• Is there any obvious bug? Are there any potential security issues or performance problems?

• Does this change need an entry in the changelogs? Does it need updated documentation?

Let’s now go into more detail about each of these points.

Good Pull Request Descriptions Document Why Some Change Was Needed

The first question a code reviewer needs to answer is if the new code is necessary at all. Adding unnecessary code has an impact on maintenance because of the introduction of new bugs, features that interact in a bad way with each other, code bloat that leads to poor performance, etc. For the reviewer to answer the question of if a change is necessary, the person who wrote the code needs to write a good pull request description. If you want to read more about the importance of good pull request descriptions, check out the blog article The Null Hypothesis by M. J. Fromberger.

A good pull request description is similar to a good commit message. The title should be short and explain what the pull request does. At PSPDFKit, we follow a specific imperative style for pull request titles (for example, “Add support for color widgets in AwesomeWidget,” and not “Added support for…,” or “Adds support for…”). The body of the pull request should explain in more detail why the change was made (with references to existing bug tickets or design documents), the approach to solve the problem, and alternative approaches (if any).

Tests Are Usually the Best Way to Understand a Feature

If, after reading the pull request description, you think the change is a good idea, you can start reading the code in the order you like. I recommend starting with the new tests, because tests are usually the best way to understand how the code works. The new functionality should be covered by tests, and the tests should follow the same engineering practices as the rest of the code. Is the new test easy to read?

If a test is added to prevent a regression after fixing a bug, it’s important to check that the test fails before the bug fix and passes after the bug fix. A good code review should focus on the tests to make sure they’ll actually catch regressions. Another suggestion is that test code should prioritize readability over uniqueness, as proposed in this Testing on the Toilet blog post.

Good Software Design Follows Objective — Not Subjective — Principles

Good software design is another important topic to focus on in code reviews. Some people treat software design as code style decisions — that is, subject to personal opinions. However, there are usually objective reasons to prefer one design over another. As a reviewer, think about what will happen if the code needs to evolve to accommodate new features. Will you need to modify a lot of files? But don’t try to look for overengineered solutions that try to solve many different problems at once. A design that isn’t too general is in most cases better, especially when the future requirements of the software aren’t clear. And if you automatize code style adherence using a linter tool, the reviewer can focus more on software design issues. At PSPDFKit, we automatize all linter checks using Danger.

I think GitHub diffs may not the best way to understand architectural changes in a code review because the relationships between code components aren’t easy to understand in plain text. Some team members at PSPDFKit prefer to download a pull request locally and inspect the code in their favorite IDE, where they can Control + click to navigate to the different parts of the codebase. In an attempt to have an even better experience reviewing architectural changes, on our experimentation/lab days, we plan to research the development of specific tooling that would help us visualize pull request changes more easily without needing to download a pull request and open it in an IDE.

Code Should Be Simple and Easy to Understand

This is, in my opinion, one of the most fundamental parts of a code review. Code is read much more frequently than it’s written, so it’s important that it’s easy to read. Focus on the names of the variables. Are they descriptive enough? A rule of thumb to follow is that the descriptiveness of a variable should depend on the scope of that variable. For instance, it’s OK to give the name i to a loop variable whose scope is narrow, but it’s not OK to name it i if it’s a class field.

Simpler code usually means code with fewer bugs and code that’s easier to maintain. If you don’t understand a piece of code, it’s probable that other people (or the author, months later) won’t either. As a reviewer, prefer simpler code to explanatory comments. However, to be actionable, proposals to write simpler code should come with concrete suggestions. When developing a new feature, it’s common that the domain is still unclear and the logic doesn’t feel as clear as it should. To avoid these kinds of situations, we found it usually helps to write some design specs before writing any code.

Automatic Tests Don’t Prove the Code Is Correct

Even if a pull request adds tests, these tests don’t prove correctness of the code they accompany. The confidence that tests add depend on the kind of tests the writer chose to write. A reviewer should feel free to suggest new tests if they think some logic isn’t correctly covered by the tests. A reviewer should also ask if they’re unsure about what the code does in a particular situation.

Manually testing a pull request shouldn’t be necessary in most cases, but it can help detect issues that aren’t covered by tests. I think that changes to the user interface (UI) of a program benefit the most from manual testing, because it’s difficult to understand a UI change by only looking at the code.

Documentation Is Key to Help People Use Your Software

Software requires written documentation before it can be useful to people who lack context about how the software works. We distinguish between two kinds of technical documentation: internal, which is used by people inside the company, and external, which is used by people who use the framework outside the company.

It’s crucial that we check how changes to the code affect the documentation we expose to customers. If not, that documentation will slowly deviate from the source code and become less useful over time.

Documentation usually follows a separate lifecycle than the rest of the code. For example, at PSPDFKit, every substantial external change to the documentation undergoes review by a copy editor. In many cases, we don’t wait for the copy review to finish before merging a code change. We mark the feature as pending a documentation change so we don’t forget about it whenever we prepare the next release of our software.

If you keep a changelog of the externally visible changes to your software, a code review should check if a new changelog entry is included and explains the change without using a lot of technical jargon.

Conclusion

Code reviews are an important checkpoint before changing the functionality of some program. They help prevent bad changes that worsen a codebase over time and are a great tool for spreading knowledge across team members. By focusing on the important things, a code reviewer will do their part to make sure the changes are really improvements over what’s already part of the program.

Here’s a summarized list of take-home lessons from this article:

• Spend some time polishing your pull request descriptions. If you’re a reviewer and the pull request description isn’t clear, always ask for clarification before reviewing the code. If you think the change isn’t a good idea, feel free to send it back before spending more time on it.

• The effectiveness of tests depends on how the writer chose the tests to write. As a reviewer, always suggest new tests if you think something isn’t completely covered. Make sure tests follow the same standards as the rest of the code.

• Good software design has a great impact on the codebase in the long term. If you’re a reviewer, always give concrete reasons about why you prefer one design over another. Feel free to ask for less overengineered solutions when it makes sense.

• If you don’t understand some code when reviewing it, it’s probable that other people will have the same problem as well. Simplify the code instead of adding explanatory comments. Having design specs before writing code can avoid wasting time during a code review.

• Even if there are automatic tests, see if there are any obvious bugs or unhandled conditions. Suggest new tests to check those conditions. If the pull request touches the UI, it’s a good idea to run the code manually to understand and see the changes better.

• Check if documentation follows the same standards as the rest of the codebase. External changes should be documented in most cases, but acknowledge that they follow a separate lifecycle that shouldn’t block merging the main code change.

In the next article of this series, we’ll focus on the code writer side of the equation, which involves things like the soft skills needed so that the writer and the code reviewer can cooperate to develop a good patch.