There are many articles written about why every software team should use PR reviews in their development process. Today I want to talk about our experience with PR reviews at Sphere, where we are a small team of 2 iOS and 2 Android engineers in a start-up environment, which means that delivering features at speed and quality is the most important.

I recently looked at number of PRs merged since we moved from Github to GitLab and I was in awe. Since 28 of May 4 people (mostly 2 since the Android team only recently moved to our mono repo) were able to merge 257 PRs. Which is around 4.28 PRs per day.

I believe few aspects contributed to efficient PR reviews:

Common architecture

We have a common architecture patterns we use, both on the Domain layer in our shared KMM library and UI layer where we use MVI powered by Loop. This means that people do not waste time trying to figure out how a feature works.

High level of trust

In many cases when I review code I do not necessarily look at the correctness of an algorithm. And even if there are bugs, we can always fix them later. Especially if a feature is under a feature flag.

Approve PRs early

We approve PRs even if there are comments to be addressed. We only request changes in cases where there is a follow-up review needed once a person addressed the comments. Which is my absolute favorite point as you do not need to waste your’s and other person’s time to ping them to approve the PR once comments are addressed.

Small PRs

Which means they can be reviewed within 1-2 min. For that, we use stacked branches. So once you feel like there is an independent piece of change that can be reviewed - you open a PR and branch off to proceed with further work.

I would also like to mention common mistakes that happen during PR reviews:

No common way of doing things.

By this I mean that it could be that team is using MVVM or MVP. But each ViewModel or Presenter is a completely different beast. That’s why not only the name of architecture is important but small patters that are used throughout it, e.g how user input is dispatched, how internal state changes are propagated to the UI, how UI is built.

Architecture review during PRs.

This is probably the most common mistake teams make. And is absolute waste of time on both sides. It’s too late to review architecture decisions during a PR! If a feature is that complex, that it will impact the work of whole team, then other ceremonies should have happened before it. Like proposals, request for comments, presentations, spikes etc.

Nit picking

I believe many people will agree they are annoying, especially when someone requests changes for them. There are plenty of tools these days that can insure code style is correct.

Large PRs

Large PRs slow down review process significantly, people are lazy to pick them up, very often review does not happen in one go.

The tone

And the last thing is the language. I strongly encourage everyone to stop using works like “I” and “You” in the PR comments. It’s all about “Us”, “We” and the team.

Written by Serg Dort, who works and lives in London builds useful things. You can follow him on Twitter