Five Pull Request Review Anti-patterns

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

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.

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.

  • 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.

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.

4. Silently abandoned review 😶

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

  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.

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.

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)

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Alberto Faci

Alberto Faci

Software Engineer | talks about software, multilingual children