Pull Requests as a Team Sport
When you’re writing code, what does it mean to be productive?
A common response is: “I’m productive when I get to focus on what I’m working on, finish it, deploy it, and take the next ticket.” This makes sense. People love to feel a sense of accomplishment, to be able to look back, point at something, and say “this is what I have achieved.”
This definition is fine if you’re coding by yourself, but what about when you start working on a team? Before you merge your code, it needs to be reviewed. When working with teams, being productive — writing, merging and deploying code — requires several people to work together.
It is very common to see the individual definition of productivity smash into the reality of productivity of teams. Individuals feel that their productivity is negatively impacted by the constant distraction of reviewing their teammates’ code, even though they know that they require their teammates to be distracted in order to get their own code reviewed.
So how can we break this mindset?
At this point it’ll help to define our ideal outcome: we need to ship code, in small pieces, frequently. Our business success depends on the speed with which we can create and update our systems. It depends upon our agility.
It’s also helpful to define the thing that we all think is getting in the way: context switching. It’s taxing, time consuming, and inefficient. It’s also unavoidable in a team setting.
So really our goal is to find a code review process that minimizes context switching while keeping us agile.
An attempt at modeling real-world solutions
There are two common processes for code reviews: parallelizing work while waiting for code to be reviewed and treating Pull Requests (PRs) as blockers. To help us understand which is best suited to our goal, let’s model both using some real-world variables.
An important caveat:
Neither solution models what happens when PRs require additional commits. Frankly, it was just too hard to model (some extrapolations can be found below).
Solution 1: Parallelizing work while waiting for code to be reviewed
A person finishes their code and creates a pull request. At that point they check for any PRs that need their attention, review as needed, then start another ticket while waiting for one of their teammates to review their code. When their code is reviewed, they either merge it or make updates and wait for it to be reviewed again.
In this solution we deploy 6 times, but we don’t start deploying until nearly halfway into the sprint. We see a pileup of review and deploy tasks for those working on small tickets, and significantly less review and deploy tasks for those working on large tickets. We see 2 tickets fall over and one not get started.
The goal of parallelizing is to optimize for individual productivity by reducing distractions and context switching. The reality is that those distractions still exist. As we can see in the model, this way of working tends to demand more reviews from people working on smaller tickets, and less from people working on larger tickets. This means that if complex tickets are handled by the more experienced developers, the less experienced developers are the ones responsible for reviewing most of the code.
This solution requires more than one person to finish their code before any code can be reviewed. If tasks are of a similar size, this is fine, but if they’re different, a second or even a third ticket can be underway before the first is deployed. This means that context switching occurs for both the person reviewing the code and the person revisiting their PR.
Because reviews pile up, this solution can also lead to multiple deployments in a short time period, which leads to increased risk — it can be difficult to understand which release caused an issue, which makes rolling back less effective.
Solution 2: Treating code reviews as blockers
A person finishes their code, creates a pull request, and immediately notifies their team. The first available teammate stops what they’re doing to review the code, and the reviewee remains available for their comments. What follows is generally a short flurry of conversation (either IRL or in the PR) and either an approval or a request for changes. The reviewer then gets back to their ticket, and the process is repeated until the code is merged.
In this solution, we start shipping code very quickly, and continue to do so regularly throughout the sprint. We deploy 7 times, 1 ticket falls over, and two don’t get started.
The goal of Solution 2 is to optimize for team productivity. It prioritizes the individual finishing a ticket over the individual part-way through a ticket. This means that tickets take longer, but they are reviewed and deployed immediately after the code is completed.
So which should we choose?
Let’s revisit the first part of our goal: find a code review process that minimizes context switching.
In a perfect world, Solution 1 only requires context switching when a ticket is completed. This is true for a brief period of time, but falls apart once the developer revisits the pull request they wrote for their second-to-last ticket. Remembering what you were thinking two tickets ago is objectively harder than returning to the ticket you were working on. There are other practical concerns: what if they’ve had to install new dependencies for their other branch? What if they had to configure test data in a database? Also, since reviews happen asynchronously, Solution 1 asks both the reviewer and the reviewee to context switch (albeit only once they’ve completed a ticket).
Solution 2 asks the reviewer to context switch out of the ticket they’re currently working on and then return to it. This is objectively easier than what is asked by Solution 1. Since reviews happen immediately upon completion, it doesn’t ask the reviewee to context switch at all.
Now let’s revisit the second part of the goal: keeping us agile.
Solution 1 can lead to bursts of deploys, and tends to have a lag before deploying because of asynchronous reviews. Solution 2 has us ship code as soon as possible after it is completed. Agile asks us to ship early and often, which means Solution 2 is better suited to that goal.
Let’s remember that we didn’t attempt to model PRs that require additional commits, but we can extrapolate that Solution 2 would look pretty similar, and Solution 1 would have delays between the updated PR and the team’s ability to review it. Ultimately, in the (very real) solution where PRs require updates, Solution 2 is more efficient than Solution 1.
In conclusion
Coding is a team sport, and we need to build processes that champion team output. We measure our productivity based on the changes we deploy, not the code we write. If we treat reviews like blockers we’re shifting the priority from individual work to team output. Good teams create work that is better than what they can create individually, and this process gets that code into the wild as quickly as possible.
If you are interested in learning more about Bench Accounting or a career with our Engineering team, then please visit us at https://bench.co/careers/
