I've argued before that I believe pull requests are the way to go, and one of the best ways to rapidly level up your organization by providing constant fast feedback.
That said there are a number of ways that pull request reviews can fail.
- Ain't nobody got time for that
Code review isn't seen as something that's valuable or required by the organization, so nobody does it.
As an engineering leader if you've opted to adopt pull request reviews, you need to make sure you've explained why they're important to individuals, the organization, and how everyone benefits from great reviews. Most importantly you need to build in time for reviews so folks feel they have time to do them well.
- Look how smart I am
I need to prove to everyone that I'm worth having on the team by being a jerk to my team to prove my intellectual and engineering superiority.
As an engineering leader just let these folks go. If you're not in a position where you can make that call, do everything in your power to avoid getting reviews from these individuals.
- If we never ship we can't ship bugs
The organization is so scared of making a mistake that pull requests can be used to stop shipping, and therefore you never ship a bug.
Shipping bugs is part of the game. The important part is being able to recover from them quickly, whether through feature flags, canary deploys, or being able to roll back quickly.
- Everything is a nail reviews
Pull requests are only reviewed for a single type of defect. When all you've got in your review toolbelt is a hammer... The worst aspect of this is only looking at formatting, because that can be covered by a linter.
You can beat this problem by having a list of things you should cover as a reviewer, and automated checks.
- Rubber stamping/LGTM reviews
Reviews are required by my organization, but rather than take the time to do it well, just sign off so I can get back to coding.
Just like number one, make sure the team understands and experiences the value of reviews as quickly and often as possible. Chat with individuals that are repeat offenders about how this hurts them and the organization, by preventing learning and growth.
- The review that never ends
Endless rounds of comments are left, without a clear end in sight. Often combined with just ghosting the author.
Hey, could you change this?
... And this?
... Could you rebase too?
... Oh yeah, just one more thing.
As an engineering leader make sure your team understands they need to set clear expectations around what changes need to be made for an approval to be granted. As an engineer experiencing this, reach out to the reviewer outside the code review context and set up some time to discuss the pull request synchronously.
- The block and ghost
Someone requests changes on a pull request, blocking it's progression, and then disappears for days. Whether heads down or on vacation this effectively destroys the author's ability to get their code merged.
When blocked by someone with a history of disappearing, reach out to them directly to get the issue unblocked, outside of your pull request review interface of choice. Work with the team to ensure they understand and empathize with how frustrating this is.
- The sock puppet
Where a senior engineer nit-picks junior or mid level folks until they might as well have written the code themselves, but in a tedious and morale destroying way.
As an engineering leader, work with your team so they understand this isn't productive, and frustrates members of the team. As a senior engineer, think about if the changes you're requesting are impactful, or if this just doesn't look exactly the way you'd like it to look?
- Too many cooks
In an effort to be super safe, or no one counting how many folks are on the team, the number of individuals required to approve a pull request makes no sense. For example requiring 2 approvals on a team of 3 engineers.
If you can't control this, voice your concerns with your manager. If you can control this, adjust the approval count to something that makes sense.
These failure modes can often be found together, and can be combined in interesting and novel ways, but they are all overcame in similar ways, building an organization where the purpose of code review is understood and individuals trust each other to have the best interests of the team in place.
Inspired by a discussion in the Rands Leadership Slack.