How to review PRs

Pull requests are an essential aspect of our way of working at S19. They are powerful tools for ensuring the quality of our codebase and a great way to make sure we stay aligned with our work. This mandates a need for consistency across our reviews. Some pull requests might have lots of nitpicking comments, while others might be approved without any requirement for change. It might be based on the reviewer, the developer might be in a hurry, so she might be forced to approve without having time to properly review the PR.

In an effort to extract the main ingredients for conducting an effective pull request review, I’ve highlighted three key points to keep in mind when reviewing a pull request: understanding how critical a feature is, conducting a thorough analysis while staying flexible, and providing feedback in a constructive way. Following these rules will help developers make the best out of PRs while fostering a constructive collaboration.

Judging how critical a feature is

Why is it important to judge how critical a feature is? Just as development can be done in a multitude of ways, code reviewing is not an exact science. I generally rely on two cues to decide how I will be reviewing a PR: pattern uniqueness and durability of feature. They will directly impact the speed of the review, as well as the amount of comment and question I leave in a PR.

Pattern Uniqueness

When reviewing PRs the first thing I do is to quickly scroll through the whole list of changes, and in about 20 seconds I can get a very good idea of how long this review is going to take me. That’s because I generally look for patterns, and structures similar to the ones I have worked with.

If something is consistent I can easily skip a lot of the mental processing and understand quickly what is going on. It might be that the types used are consistent, the method signatures or the ways modules are created. Even if it is less efficient, consistency will generally yield better results from a developer’s point of view because it makes a codebase easier to understand. When the patterns used are similar, we can easily infer what the result will be.

At a moment or another, we will be brought to using different patterns in our code. It might be a new authorisation library, or just a different way to write functions. On one hand those new patterns will increase code complexity, and will require the reviewer to spend extra effort in understanding what is happening. On the other hand, this is also a way for the reviewer to learn and gain knowledge at a very fast pace, considering the amount of time such feature could have taken. New patterns should also be reviewed with extra scrutiny as they influence how the codebase will evolve.

Durability of feature

At S19, the majority of our projects focus on innovation, quick iterations, and prototyping quick ideas — all of this without specially requiring the need to scale. At the same time we also build pieces of code which are meant to be used throughout our products, these modules are much more critical and require much more attention. It is essential to identify how durable a feature is meant to be, for that we need to be aware of the level of advancement of our project in its lifecycle. Is it going to be deployed to tens of thousands of customers, or will be be restricted to a selected pool of testers?

A non-durable piece of code might be when we want to quickly test out a capability. For instance there is a new peer-to-peer database we want to test, and it has to be shipped as part of our prototype. Our projects being highly de-risked, we should be able to deploy a potentially fatal piece of code. In this case downtime is not an issue, there will be no need for maintenance and the code will be rewritten once we have validated our hypothesis.

Most code changes will be about enhancing the set of features of the product: they are additional capabilities that function as coded by the programmer. The larger part of the work of a software engineer consist in delivering features. It might be in the context of a sprint: when delivering a feature we are bringing a project closer to its goal. In this case the review is more critical as it must take into consideration the objectives of the project/sprint. In order to have awareness of the context, it is useful to know about the goals of the project and to question how is the PR working towards this.

Whether it is a for particular HTTP service, or for a library specific to a repository, we will often have to work on key reusable components. These require particular attention since they will dictate the way future code is written. In this case the reviewer should understand how those components will be used, what now becomes under scrutiny is the API of what was built rather than the code itself. Because we are talking about such a key component, it might be best to involve multiple people in the review process.

Be thorough while staying flexible

Reviewing a large PR can often be a daunting task, however it is an essential part of the development process. A good reviewer will be involved in the PR while letting the original contributor make the decisions.

Make an effort to understand how the feature works

It’s often easy to just roam through the list of changes looking for simple mistakes, spelling errors and so on. That’s what the linting, types and tests are here for. The main difficulty is to overcome the temptation to read aimlessly throughout the PR and approve it without having understood the logic behind it. A good reviewer should jump up and down the list of changes to make sure she has a good grasp of what has happened, the entire execution of the program should be well understood.

Being opened to different ideas

As a developer perfects her skills in a particular domain, she will inevitably start to prefer a particular set of patterns and paradigms. For instance one might prefer a functional style versus a more imperative style. Unless some patterns have been approved by the team as standard across the codebase, the reviewer should stay opened to innovation and diversity in the code. It is sometimes difficult to differentiate between a piece of code which doesn’t respect an established pattern, and just a different way to solve a problem. Keep in mind that being able express oneself in code is one of most enjoyable aspects of being a developer, and unless a particular strategy has been declared the de-facto solution, we should stay opened to multiple solutions.

Provide feedback and ask questions

Well conducted code reviews enable both parties to learn by using the PR as a communication tool. Whether a developer is junior or more senior, it will benefit both. The trick is to use code reviews as a way to learn, and to strengthen the team by tacking hard problems together.

Be nice

When spending hours working on a problem, you don’t just write a solution. You write the best solution, it might not be the most efficient, or the most engineered solution, but considering the context, your knowledge and the various constrains applied on the project, to you it’s going to be the best solution. This is why it can be difficult to accept receiving negative feedback on a PR, and it can sometimes feel a bit too personal. Reviewers have to be conscious of this, and should stay as objective as possible when leaving comments. Comments should be helpful and the reviewer should use wording which can help initiate collaboration and enthusiasm.

It’s fine to ask questions

When working with senior developers, I often end-up seeing reviewing pull-requests written by people more skilled than myself. At the code level there might be some concepts that I don’t understand, or things might evolve in a direction which I hadn’t foreseen. If a quick Google search cannot clarify things, it’s completely acceptable to ask questions.

Provide a solution, be explicit

An equally frustrating experience when I have my PR being reviewed is when a single comment is left, with no more explanation than “This is wrong”. While it might be true, this is leaving the developer to have to guess what was wrong about the code. She might not be aware of the better solution, or there might be several other ways of writing it and having the developer guess it is counter-productive. Leaving a descriptive solution as part of the comment will help the conversation move forward, and it will be easier for the author to understand exactly what the reviewer expects.

TLDR;

• Understand the changes in the context of the codebase as well as the context of the project.
• Understand the ins-and-outs of the change while staying opened to different ideas.
• Be nice, be explicit in your comments, and ask questions if you don’t know.