Writing better PRs is one of the longest investments in your career as a Software Engineer. In this article, I will share some simple but effective tips that will make your PRs significantly easier to review, with live examples practiced by myself and the FormSG team. This will help your PRs be picked up for review much faster, build trust within your team and your reviewers will thank you for it.
Why is this important? How will this help me?
Getting your PRs merged in much quicker
Have you ever encountered a situation when your PRs stay "in-review" for longer than desired? Every software engineer can likely relate to the experience of asking other fellow engineers to help to review your code changes. Often, there is a implicit resistance or lag before reviews come in as it requires context-switching and takes up the reviewer's limited available bandwidth.
Making your PRs easier to review helps reduce this lag, builds trust amongst fellow engineers and helps bring your changes to users faster.
Unlocking the true value of code reviews — building release confidence
There is a common software engineering saying that big PRs often receive LGTM, whereas small PRs usually receive more in-depth review and issues are surfaced more easily. This is since looking for issues in poorly scoped, big PRs are like looking for a needle in a haystack — its challenging and often unrewarding for the reviewer to do.
If service reliability is crucial to you and you'd like to minimize the risk of production incidents, it is crucial that reviewers are able to easily spot and surface issues. For example, in FormSG, practicing writing specifically- scoped and smaller code reviews across the engineering team has led to a significant drop in production incidents due to code change.
Firstly, what exactly makes a PR easy to review?
Fundamentally, it boils down to this simple idea:
A PR is easier to review if intention of the PR is focused, clear and well-scoped (and code changes are limited to this intention).
This idea unlocks the chain of benefits:
- smaller PRs
- easier understanding and verification of the clear and limited scope by the reviewer
- improves the reviewer's ability to give quality feedback and spot issues
- less resistance to pick up reviews in the team, leading to your code being merged in quicker
- higher release confidence and more reliable services
Beyond simply counting the LoC in a single PR, PRs are often hard to review as they mix and try to include too much irrelevant scope. For example, a simple feature PR might include un-related refactors found along the way. While the boy scout rule is appreciated, these refactors should be included in a separate PR if possible.
The argument for larger scopes and PRs
Often, combining scope of PR is done as it saves the PR writer's time. It is deemed much more convenient to package every desired code change into a single PR to avoid the hassle of making multiple feature branches and commits.
However, if you consider the total engineering time due to resistance to review a large PR, potential incidents missed and the time it takes to remediate it. Writing smaller well-scoped PRs usually end up being net positive for the wider team. It also leaves a nice effect of allowing PRs to be used as a clean documentation trail for the team (using tools such as git blame).
Furthermore, writing smaller scoped PRs is a skill that can be developed. By practicing the following tips, you will be able to master this skill and achieve smaller and well-scoped PRs without compromising on velocity as a PR writer.
Learning technical recommendations with a deep-dive example
Let's deep dive into a case study using my personal work as example:
Previously, when implementing a save draft feature, I practiced the "boy scout rule" and tried to include as much changes as possible to "improve" the code. However, this led to a large and poorly scoped PR. This made it challenging to review and contributed to production incidents due to a missed useEffect reference dependency issue.
What issues are there in this PR?
Firstly, looking at the commit history — we can see some applications of this "boy scout rule".
hoist state to provider is an example, where I tried to make the state reusable. However, this is a large change and could have been split into its own PR, leading to a much more focused scope and more effective review. This ultimately led to a production incident.
Secondly, the commit history might not really be useful to a reviewer. Everything is jumbled and stray commits like feat: remove console logs add noise for the reviewer. These commits could have been merged into the offending commits.
Ultimately, we can see this is a large PR due to the desire to combine multiple intentions (hoisting the provider and implementing a save draft feature). This led to firstly, resistance by reviewers to review this massive change and secondly, missed issues during review which led to production incidents.
What did I do to make my PRs easier to review and address my mistakes?
In a subsequent feature release, I applied the tips studied in this article and made my PRs easier to review. This led to a more effective review process where issues were flagged, PRs were picked up for review with less resistance and higher release confidence.
Immediately, you might be able to tell a few differences.
- The lines of code changed is significantly smaller, leading to quicker reviews per PR.
- Each PR, in the case of the above which is a "boy scout" improvement to do some abstraction, is split and focused and easy to understand and verify.
- Each change now has its own clearly defined context and "why" behind the change, leading to clear documentation for future engineering teams.
Concrete steps into how to achieve the above:
- Breaking down a feature into multiple standalone parts
For example, if you're working on a pdf generation feature, you could break it down into parts. This limits and keeps the scope of each PR focused, reducing the size and allowing the PRs to be more easily understood and verified.
This is easier said than done. The complexity is that you might only know of the changes that need to be made during the implementation phase and cannot pre-breakdown the feature. For example, you might only know of the need to abstract after halfway working on the feature. Read more on the following git tricks in the review-by-commit section to help you with this.
2. Practicing review-by-commit
In certain cases, PRs may not be able to be decomposed further and a minimal set of changes must be merged atomically.
In this case, you can apply the review-by-commit approach described below.
For example, in the above PR, the build scripts across multiple packages need to be updated together.
As you can see, each commit is scoped and can be reviewed sequentially by itself. This can also be applied together with other techniques in this article.
How can you neatly organize your commits like above? By using some simple but useful git tricks.
1. Be selective about what to push in each commit. Use tools with VSC's selective commit.
Make sure to only selectively stage changes for commit. You can use your IDE's tooling to help with this. In the above screenshot, I specifically only staged the release_hotfix and included it in its own scoped commit feat: update release hotfix script
By being granular, you can then re-order and merge the PR changes easily in the next step.
2. Make use of git interactive rebase
The reorder and fixup command is extremely useful to merge in related commits into a single commit and reorder your commits to achieve a clear narrative for your reviewers.
Tutorial:
Suppose i have some further changes to the release_hotfix script i need to make discovered after I've made the above feat: update release hotfix script .
I would like to re-order or merge this commit with the feat: update release hotfix script commit to paint a clear narrative to the reviewer.
Hence, I will run the following command: git rebase -i HEAD~5
This maintains the commit narrative and organizes your commits. Whether to use fixup or simply reorder the commit can be decided on based on context.
Benefits
When you do review by commit, it potentially replaces the "Changes made" section of the PR body, since each commit scopes and lists the changes made. A reviewer can focus on reviewing each commit to understand if each change achieves the intended result.
Moving back to part 1, how do we split into multiple PRs if we only know of the changes needed while halfway working on the feature?
By applying the following review-by-commit structure, we can simply group commits logically into its own PR! This unlocks the idea of "stacked" PRs even for unforeseen required code changes.
The following can be done using for example:
git checkout -b PR1
git cherry-pick ^A..B
git checkout -b PR2
git cherry-pick ^C..DEach commit is now its own well-scoped and tiny PR!
3. Conventional commits
Conventional commits is a useful way to further document your commits. By specifying the type of commit, you can signal to reviewers what each change aims to do.
Furthermore, these commits are machine parsable and allow you to generate eg, change logs or semantic version bumps based on them. There is an extensive list of tooling based on this style of commits here: https://www.conventionalcommits.org/en/about/#tooling-for-conventional-commits
For example, the FormSG team uses our conventional commit history to generate changelogs and semantic version bumps. Example here: https://github.com/opengovsg/FormSG/pull/9193
4. Writing a clear PR body and leaving comments on your code for gotchas considered
Minimally, every PR body should very clearly describe the problem that this change aims to solve. This is the first step reviewers usually need to answer which is "Why is this change even being introduced?". This problem should be as focused as possible.
For specific changes with special gotchas or tradeoffs, consider adding a comment in your PR on that specific LoC to explain the "why" behind the change.
For example, looking through your own PR and writing down "why" a change is made: https://github.com/opengovsg/FormSG/pull/9193#discussion_r2944167373
Results from making this change
After implementing the following simple methodologies, I found:
- that reviews on my code have been much more effective, more issues that would otherwise be missed are flagged
- release confidence has been elevated
- my PRs are also picked up for review much faster, leading to higher release cadence and more impact to our users.
References and credits:
- My fellow FormSG engineers for being the inspiration for some of these learnings. View our open source code base here: https://github.com/opengovsg/FormSG