Improving review time for Pull Requests

In order for the Code Review step to not block development, we need to both prepare the code for a code review so it’s easy to parse and review, and — on the reviewer’s side — to do so quickly.

TL;DR:

• Prepare the pull request for easy consumption by making it small, making the problem and solution clear, including which parts are the main points to review (the more contentious parts), and making it clear what stage the review is in (idea/WIP/ready to review)
• You can improve Time To Review by splitting the Structural review (“doing the thing right” — readable, consistent, error handling) from the Domain review (“are we doing the right thing”), as the second usually requires approval from specific people

Preparing a Pull Request

Small PRs

Pull Requests should be as small as possible for a self-contained functionality. Changes that are independent of one another should be split into different PRs.

This lessens the scope, both making reviewing easier and avoiding blocking other independent changes.

Easily parseable

The title of the PR should tell the reviewer as much as possible what problem we’re addressing and what the proposed solution is.
This is mostly repo-specific, and can be done for example by:

• Starting with a Conventional Commit prefix (feat/fix/docs/test/ci/chore)
• Contain the Jira ticket (e.g. TAPI-1234)

The extra 10 seconds that go into defining the PR name allow the reviewer to get a clear idea of what the PR is about and why, even without entering the PR itself

The description of the Pull request should contain a more detailed version of the same — what the current problem/goal is, and how we’re attempting to solve it

Main points to review

A lot of code is boilerplate — ‘necessary but not interesting’. For the Code Reviewer to be effective, we need to point out areas of possible contention.

A simple way we can do this is by clarifying non-trivial decisions made in this PR, and what alternatives were considered.

Work-in-progress vs Ready for Review

Often, you may open a PR even though the code isn’t production ready — as a Request For Comments, for the CI tests, etc.

Github allows to create PRs as “Draft” PRs, or to “convert to draft” (under “reviewers” section). This allows a visual distinction between places where the review is blocking code from entering the repo vs places where it is not.

Reviewing Code — Domain vs Structural reviews

There are two kinds of problems to review in Code Reviews — Domain problems (are we doing the right thing?) and Structural problems (are we doing the thing right?).

Usually there are very few people who can do the Domain side of the review, and many more who can do the Structural review.

To spread the load more evenly and get faster turnaround time on reviews, we can ask people without domain knowledge to review the structure of our code (“given that this is the solution, are we implementing it well”), leaving the domain expert to weigh in on the correctness of the solution.

You can approve specific parts, stating in the review that e.g. you have only approved that the code is readable and logical, not that this is the correct solution.

Is the code readable?

• Meaningful names to functions/variables
• Clarity of logic
• Inputs, outputs, and variables are Typed where possible

Is the code correct?

• Handles edge-cases for input/state
• Handles failures (for example, every external call to a DB or service may fail — if so, what happens?)

Does the code conform to team/repo standards?

• Tests
• Code conventions
• Logging

Domain review

Where and how should data be saved?

• Who is responsible for this information? Where does it come from? When can it change?
• What format does it come in?

Our responsibility in Inter-service processes

• What do we expect to receive as input?
• What should we be sending as output?