Five Pull Request Review Anti-patterns

https://twitter.com/albertofaci

A Pull Request (PR) is the process in which a developer submits a body of changes to be integrated in the main branch of the code base. This is the last chance for other team members to highlight issues, make comments and request changes. Every organisation implements a different processes, but typically a number of peer approvals are required, as well as a working build. Other policies may exist, such as “code owners” as gatekeepers for specific repositories of code.

Continue reading to learn about:

1. Reciprocal leniency
2. Inconsistent feedback
3. Latecomer to the party
4. Silently abandoned review
5. Approved not even looking at the code

This process relatively standardised across SCM platforms (GitHub, BitBucket, etc) and originates from contributions to Open Source projects with disperse collaboration, low implicit trust in the author and high quality requirements.

One could argue that providing an in-depth review after the code has been written might be too late, and the solution and implementation details should have been agreed in consensus. This means that the purpose of the PR changes across organisations. In some teams the PR is a simple confirmation that the changes have been implemented as discussed; in other teams, the team members are hearing about the details of the solution for the first time.

In this article I’m pointing out some anti-patterns I have observed in Pull Requests and general code reviews in my experience.

1. Reciprocal leniency 🤝

I have observed some interesting people dynamics occur around the process of requesting and performing PR reviews on a piece of code.

Developers who are active and insightful in their reviews receive attention from fellow developers when they raise a PR. Conversely, developers who are less proactive in their reviews often end up begging for reviews to avoid lagging behind and getting in merging hell.

You scratch my back, I’ll scratch your back.

This is specially bad if this favour bartering starts including being too lenient in the reviews to grant approvals fast with the expectation that the same will occur in PRs where they are the author.

2. Inconsistent feedback 🗣️

This goes with personality and experience of the reviewer. Some engineers will take the effort of checking out the code, examining it in the IDE, running the tests locally, etc. This will provide much better insights than a review done solely on the web diff tool of the SCM platform.

Some issues in this category are:

• Nit-picking: superficial comments missing the forest for the trees. Detail such as whitespaces and formatting.
• Pointing out issues outside the scope of the changes.
• Getting in refactoring rabbit holes for the sake of code beauty.

I think you could do all that in a oneliner

3. Latecomer to the party 📢

After not paying attention initially and once the author has addressed most of the comments raised by the reviewers, the latecomer turns up (often with an apology) and requests deep changes that often overlap and invalidate fixes from other comments.

I hope it’s OK if I suggest a couple of changes, if it’s not too late

The consequence of having a latecomer is a delay in merging the changes and wasted effort. Unfortunately this usually one of the senior members of the team, who is as busy as opinionated in coding style.

This can occasionally mean a fundamental change in the solution which may invalidate the entire PR.

4. Silently abandoned review 😶

Also known as “ghosting” if the author had asked for a review directly.

In this anti-pattern the reviewer opens the PR, lurks around and decides that:

1. They cannot invest the time in making a deep review to provide insightful comments, and
2. They won’t mark as approved without providing a proper review.

These are not bad things per se, but are a symptom of an engineer not taking the effort or the process seriously.

5. Approved not even looking at the code ✅

I personally think that the SCM platforms should collect metrics on two things:

1. Time taken by reviewers from first opening the PR until ticking as approved.
2. Number of files not opened in the PR diff browser.

I have often received approvals within seconds of requesting a review. This is both a lack of respect (even though the intention is the opposite) and a complete disregards to the principle of accountability that should apply equally to a change author and its reviewers.

The exception to this is having pair-programmed with the reviewer. In that case it is acceptable since the reviewer has participated actively and the four-eyes review principle is upheld.

Conclusion

A recommendation on guiding principles:

• Review the code timely and in depth as you would like yours to be reviewed. (Respect)
• Consider an approval as becoming co-author of the change. Uphold the standards of your team. (Accountability)
• Concentrate your comments to the scope of change. Raise other changes separately as technical debt. (Focus)
• Be more of an engineer than a scientist. The customer gets value from closed PRs, not from long open PRs striving for perfection. (Pragmatism)