Best Practices When Doing Code Reviews — Part 2

In the first post of this series, we presented some general goals of code reviews in software engineering, and we offered some tips for code reviewers. In this second part of the series, we’ll focus on some more tips, but this time for code writers — the people who want their code to be part of the main repository.

A Governing Principle: Trust the Code Writer

A code review is really the cooperative work of at least two people: the code writer, and one or more code reviewers. If we give equal power to each party, we risk that a high number of pull requests (especially the complex and controversial ones) will stall for a long time until people reach a consensus. This is what usually happens in the peer reviews of scientific articles, for example, where both parties are independent and don’t have a common goal other than “advance science.” In a software organization, in contrast, there are typically more shared goals. And additional stakeholder dependencies put — in practice — a limit on the time for reviewing code and addressing feedback.

That said, one way to prevent the “stalled pull request” problem is to give slightly more power to the code writer. This means that if there’s a controversial decision to address in a code review, the code reviewer should give slight preference to the way the code writer thinks or implemented something, especially when there’s no clear and strong evidence that it was a bad decision.

What a Code Writer Should Focus On in a Code Review

If code reviewers should focus on things like the purpose of the code, if it has tests, the general design, etc., code writers should focus on making those things explicit to ease the work of reviewers. What follows is a list of tips that code writers always try to follow at PSPDFKit.

Write Good Pull Request Descriptions

In the first article of this series, we explained the importance of pull request descriptions and how they help guide the code review process. From a code writer’s perspective, we recommend you spend time writing good pull request descriptions. The pull request description should explain why a change was needed, and it needs to include a general overview of the implementation approach, particularly if it’s complex. Don’t forget to include links to related material, like other pull requests (if the change is a part of a series of related changes), design documents, proposals, or UI/UX documents.

Important Design Decisions Should Be Discussed before a Pull Request

One important aspect that code reviewers focus on in a code review is software design. Good software design helps maintain and extend the software in the future. However, complex designs should be discussed before the implementation is in code review. Presenting a significant design (like the design of a core component) implemented in full in a pull request creates an unnecessary tension between the code writer and the reviewer. The code reviewer will feel inclined to not challenge any substantial design decisions, because that’ll imply the code writer must rework the implementation. And even if the code reviewer suggests an alternative design, the code writer will tend to not accept it or try to negotiate a not-so-ideal solution with the code reviewer. That’s because the already implemented solution becomes a sunk cost from the point of view of the code writer.

Instead, try to discuss complex or critical software designs in advance with your colleagues. That will improve the feedback you receive, and it’ll greatly simplify the review of the pull request.

Pull Requests Should Be Small

Large pull requests usually take time to review, and that discourages code reviewers from reviewing them promptly. Also, people typically provide feedback on large pull requests that’s not as thorough or that overlooks some parts. To prevent that, code writers should try to write small pull requests.

How small? As small as they can be. It’s common at PSPDFKit that a pull request with a bug fix is just around 100 lines long, or even less. For new features, it’s common to split the implementation into multiple pull requests. Each of them should typically be smaller than 1,000 lines in length.

If you have problems splitting a large implementation into multiple small pull requests, here are a couple of tips:

• If you need refactoring before implementing a new feature, make the refactoring a separate pull request, if possible.

• Follow these tips to understand Git and improve the way you handle stacked pull requests in GitHub.

Be careful not to make your pull requests too small! Pull requests should be self-contained: They should always have a complete part of functionality, plus tests.

Accept Code Review Feedback Positively

It’s a matter of fact that every pull request, except perhaps the most trivial ones, will receive some review feedback. It’s important to take the reviewer’s feedback positively. Always assume good intentions from your code reviewer; they want the best for the product as much as you. Also, don’t take feedback personally. It’s about the code you wrote and not about you as a person or your skills as a programmer.

However, there are cases where the code writer and the reviewer need to have some back and forth discussion about a specific aspect of the code. As a code writer, you should try to reach a consensus with the code reviewer. Separate between what’s really a blocker and what can be fixed later. But don’t abuse the “can be fixed later” basket, especially if you don’t create follow-up tickets to address those concerns. Experience tell us that those “to be fixed later” things rarely get fixed. If you’ve discussed important design decisions in advance, as recommended before, it’s much easier to reach consensus. It’s also a good thing to not get too attached to your implemented solution. As mentioned before, this is easier if you don’t spend a lot of time implementing something before putting it in code review.

If, even after following these tips, you cannot reach a consensus with your code reviewer, feel free to discuss the problem more broadly with the rest of the team and document the outcome you decided on in the pull request.

Conclusion

Code reviews are one of the engineering best practices that every software organization follows. However, the way organizations approach them varies a lot.

In this series of two posts, we explained how we prefer to approach code reviews at PSPDFKit — first from the point of view of the code reviewer, and then from the point of view of the code writer. Any engineer will play one role or the other during their work day, so it’s important to know how to play them effectively so that code reviews are fluid and don’t unnecessarily delay the introduction of new features and enhancements to your codebase.