This content originally appeared on Level Up Coding - Medium and was authored by Ryan von Kunes Newton
When I first joined my current company (Gusto), I was surprised at their Github approval policy. As long as you had a single pull request approval, you could push additional changes and merge to the main branch without needing another approval.
At previous companies I’d worked, re-approval for new changes was required, so this seemed unusual. However, to my surprise, this looser policy gave us higher velocity, smaller PRs, and better code quality. Here’s why:
Code reviews are expensive
Requiring anything is always a blocker to moving forward. This is true with PR approvals. If a PR is sitting around waiting for an approval, that’s time it’s not ‘living’ with the other code or creating value for the customer.
Context switching is expensive for reviewers. If they are in the zone, they will likely wait until the end of that concentration period to review a PR. This in turn leaves the author waiting. Even the context switch for a simple review or rubber stamp can be costly.
But code reviews (often) add value
While code reviews are expensive, they can add significant value. There are 3 areas of feedback I look for:
- Get feedback on a bigger idea or strategy
- Get feedback in an area I’m not so knowledgeable about
- Get feedback on minor patterns, preferences, or conventions
About 80% of the PRs I author fall into the third category, so I’m usually just looking for an approval with some minor feedback. Sometimes I’m not even looking for any feedback, just an approval if the PR is basic enough.
Only the first two areas should be considered block worthy. And then only if there is something major that needs to change with them. If the PR is good enough to ship, then I will leave my feedback but still give an approval.
I trust the author to evaluate my comments and make their own judgment call as to whether to make the suggested changes or not. After all, the author knows the subject matter of the PR better than I do.
Leaving feedback with an approval leaves the author unblocked. They can select what comments/changes they agree with and then merge without having to request a re-review (although often they do if the changes are large).
What about bugs? Tests and CI have caught more bugs for me than any reviewer ever has. The purpose of tests is to catch bugs, which they do well. The primary purpose of reviews should not be to catch bugs, but to provide feedback on strategies and ideas.
This process is largely based on trust. My team and I share the same goals and have built up trust over time. I rely on them to make the correct decision for the team and company. I can’t be involved in every decision otherwise there would be no productivity.
What happens with stricter policies?
Let’s talk about what happens when you add stricter policies such as a re-approval after changes, or approvals from multiple parties.
As discussed above, PR reviews are expensive. As a result, the cost of merging a single PR becomes higher.
Normally I ship small PRs that do one thing. Often when building a larger feature, a PR might be dependent on the PR before it. Since the cost of shipping a single PR is higher this becomes harder and there is a risk of PR traffic jam. As a result, I’m incentivized to ship larger PRs that do more than one thing. This causes all sorts of issues:
- Larger PRs are harder to understand and review. More issues end up falling through the cracks.
- Larger PRs take longer to review. It’s more of a context switch and reviewers will be more hesitant and wait longer to review. Think of your eagerness to review a 10 liner vs a 500 liner. This further slows the development cycle.
The higher cost of merging a PR leaves me less likely to implement feedback. If there is minor feedback which I agree with, but is not mission critical, I have to weigh the cost of making that change versus shipping as is. If the cost of waiting is higher than what’s gained from it, I’m going to ship it.
This results in worse code quality. I can point to countless examples of PRs where I implemented basic suggestions because it was cheap to make those changes and merge, but would have not if I needed an additional review afterwards.
Conclusion
Adding any blockers to shipping will slow product development down. Often these blockers are necessary. But sometimes the intent of a policy has the opposite effect. Every organization should look at its policies and reduce blockers as much as possible while still being able to operate.
Author’s note: this piece was inspired after reading No code reviews by default. Gusto has since moved to a stricter review policy for regulatory compliance reasons (a net positive move for the business, even if it comes with the drawbacks I describe above).
This is part of the my Practices to rapidly ship new software products as an engineer series.
Why you should keep loose code review policies was originally published in Level Up Coding on Medium, where people are continuing the conversation by highlighting and responding to this story.
This content originally appeared on Level Up Coding - Medium and was authored by Ryan von Kunes Newton
Ryan von Kunes Newton | Sciencx (2022-04-01T13:21:01+00:00) Why you should keep loose code review policies. Retrieved from https://www.scien.cx/2022/04/01/why-you-should-keep-loose-code-review-policies/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.