Code review sucks - and it's your fault
and you are choosing for it to be that way, every day.
Just use Pair programming or Ship/Show/Ask, duh.
Sadly, the vast majority of us are stuck with Pull Requests (PRs) for code review. That’s the reality. Some are in better places than others, of course, but if you think PRs suck or you are annoyed with the process, it’s probably your own fault. It doesn’t have to be that way - and it is in your power (yes, even - or specifically - as an individual contributer) to change it.
Feature X
Development Time: 2 days
Waiting on PR: 3 days
1st Review: 30mins (elapsed time until next step, 1 day)
Addressing PR comments: 0.5 days
Waiting on Re-Review: 1 day (if you’re lucky)
Re-Review: 15mins
Approved
Total time spend working: 2.5 days ish
Total time spend waiting: at least 5 days
If this looks painfully familiar, then this article is for you; while the example may look extreme to some, it really isn’t to others.
Why does it matter?
It’s just the cost of doing business, right? Code must be reviewed - that’s how it’s done. Yes, it’s slow, but it is (likely) not the bottle neck to faster delivery (because we’re shipping only once a month anyway..) and as Gene Kim famously said:
“Any improvements made anywhere besides the bottleneck are an illusion.”
So why do I care about code reviews, if it doesn’t even improve the overall system?
Because there are other benefits to having efficient code reviews, aside from the overall system flow. Being less annoyed and more happy at work, for one.
Fast Feedback
The code you write has exactly zero value while it is sitting in a branch and hasn’t at least been seen by someone else. The further along the value stream it progresses, the more valuable it gets (it also gets more expensive to change and fix, by the way). The first step is to have it looked at by someone else. Suddenly it’s a feedback loop. An opportunity for learning. And while we say that customer feedback on a change is the most valuable, getting other feedback (like from a colleague during code review) along the way is also valuable - just in different ways (knowledge sharing, improving the health of the code base, getting diverse opinions on solutions, edge cases etc).
Turn this thought up to 11 and you actually get to Pair programming, by the way. The fastest possible way of getting feedback and other such benefits - but you’re probably here because that’s not an option for you.
Reduced context switching (and WIP)
Plenty of literature exists on why context switching and WIP (Work in Process) are bad - and connected. In the above example, chances are, you already started on a new feature after the first step. Which means you have to context switch to address PR comments. Depending on the severity of comments and size of the change set, this can get quite crippling. This is also a straight forward example of increased WIP. Your feature is not finished until it is tested and integrated (if your tests are fully automated, great! If there is a separate QA step along the path.. that’s a rant for another day). Not finished means it is WIP. You can’t focus on feature Y, while you still have to go back to feature X for little fixes here and there.
But it gets worse. You’re not the only one context switching and increasing WIP. Your reviewer is, too. Reviewing PRs requires a fair amount of focus to do well - otherwise, you just get a ‘lgtm’ (‘looks good to me’ - or the slightly more cynical version - ‘let’s gamble, try merge’) - which helps nobody. So your reviewer, having to go back every time you’ve finished addressing comments, which may be 2-3 times, is commiting a considerable amount of cognitve load to get your code over the line.
What can you do?
So, we agree that this way of doing code reviews is not ideal. Feedback isn’t fast, flow is disrupted left right and center and cognitive load is high.
Interestingly enough, there is no contract (social or otherwise) that states that PRs have to be slow, asynchronous and annoying. You are making that choice every day.
What you can do, is change your own behaviour. After-all, until the code is integrated, you are responsible for it - it’s yours. Throwing it back and forth over a fence via asynchronous github comments is not the responsible thing to do.
Instead, try having a conversation.
“Hey, can I walk you through my code for feature X after stand-up? The first draft is finished and it’d be great if we could review it together and get it merged.”
Yes, I’m suggesting Pair review (it’s like Peer review, but better). And yes, that means you’re going to have to talk to someone - shock horror. They were going to review your code eventually anyway, and there is little to no benefit to you starting something new until feature X is integrated. Might as well do the review together so context switching is a one-and-done activity, not a back and forth.
This comes with benefits:
Instant context for the reviewer;
sure some of the decisions in your code may be due to lack of experience, but some of them may be four hours worth of figuring out the only way to avoid breaking edge cases. Discussing this during the review is much faster and less painful for both of you;You can address comments right there and then (now you’re kinda Pair programming…), reducing or eliminating the need for re-reviews;
Code should be able to get merged at the end, so everybody can focus on the next thing; and
If it isn’t ready for merge, there may be some other/additional problems at play which you can easily identify - the PR might be too large, for example. Quite common - as you don’t want to do PRs too often (since they suck and are slow), you cram too many changes into the PR, which makes them suck even more and makes them even slower.
Unsurprisingly, all of this also applies if you are a reviewer. Annoyed that your PR comments and suggestions are ignored? Or it takes three reviews for them to get it right? Talk to them (nicely)!
So... still Pair programming, huh?
Well… not quite, but almost. It’s undeniable that working on something collaboratively is way more powerful than working in isolation and throwing stuff over fences.
Not everybody is immediately willing or able to Pair program, though. Co-creation, including Pair programming can be quite a personal activity and requires (and creates!) psychological safety and trust.
Pair review may well be a lower barrier of entry for co-creation. You don’t have to immediately lay bare your problem-solving process and all the mistakes you’re making during the development process, like you would during Pair programming.
You are presenting the same code the reviewer would have seen anyway - and you are able to immediately explain awkward lines or solutions, without having to worry about the reviewer pulling you up on it.
As a result, this much less severe act of co-creation will help build trust and psychological safety over time, while not requireing a lot to start with.
It’s also less of a time commitment and fits into pretty much any development environment or framework (even those managers who think that two people working on the same ticket is inefficient can hardly find fault with visibly faster and more efficient code review).
The beauty of Pair review is that you can start this today - without having to get permission or having long discussions about it. You can choose not to throw your PR over the fence, but to take ownership of your code and get it finished. And if you find it works well for you, you can always tell the team at the next retro (your reviewer(s) might have a positive thing or two to say about it, too!) Maybe one or two more people will join in (or not, who cares as long as you’re no longer annoyed by your code not being reviewed in a timely manner).