Better PRs

I posted this article on my current employer's internal wiki

Motivation

Why do we want our changes to be reviewed by other members of our team?

There are multiple reasons, but for me, the primary ones are:

• Knowledge sharing. We want more people to understand the codebase and be able to make changes to it.
• Solution validation. We want multiple engineers analyzing our solution and trying to find problems with it, suggest alternatives, etc.
• Increase quality. We want to identify errors, omissions, inconsistencies, inefficiencies, etc. as early as possible and fix them before they reach production.

Proposal

As individual contributors to the source, we need to foster these goals, by spending time/effort on improving our PRs and guiding reviewers through them!

Ways to improve PRs:

• keep PRs as small and self-contained as possible
  • minimise the changeset! Eg. don't reformat source, don't remove whitespaces, don't move code around, no refactors unless relevant, etc.
    • All these are useful but should be done in separate, specialised PRs.
  • This makes PRs manageable and keeps reviewers focused!
• your PR description should answer two questions:
  • Why? - what is the motivation for this work. What is the problem the PR is solving?
  • How? - a short guide on the solution implemented in the PR. More on this next.
  • This builds the context the reviewer needs to do their job. It's OK if this is a link to the project management tool, as long as the story description answers these questions.
• short, self-contained commits with clear and concise commit messages.
  • This encourages reviewers to go through commits one by one, which is an intuitive order to review code.

Guide reviewers

• PRs usually have one core change, everything else is in support of that change - eg. new utils, new types, new tests, etc. If that core change is not obvious, make it explicit in the description, have the reviewer start with that!
  • This gives structure to the review
• Guide reviewers to where the "tricky" changes are. Have them focus on these changes to validate your approach.
  • Good candidates are: a SQL query whose performance you're not sure about, a test whose coverage can be improved, an algorithm you implemented where you're not sure about the edge-cases.
  • If needed, describe what were the alternatives, why you choose that option, what are the advantages and disadvantages, etc.

Conclussion

This is significantly more work! I hear you say.

I agree, but I think that with practice things will speed up. The benefit is that on a medium/long term it will make us - as a team - move faster and produce better work.