Sometimes in the following I sound like i’m stating facts. Please infer an ‘in my opinion’ in front of everything you read here. If you disagree, awesome, let me know because I’m interested. I’m definitely not trying to prescribe truths here but just speaking from my experience.
In the grand scheme of things, I’m still quite new to my career in software engineering. I’ve been working in the field since 2013, at four separate places during that time. Three of those places had some form of code-review as part of standard process. One, a bank that performed code review as part of the process after code was on SVN mainline. Two, where code-review was a strict process where discussions could go on for hours before your code ever hit the master branch.
When you join these places, part of the engineering onboarding is usually talking about how one goes through the process of PRs (Pull Requests). In particular it is mentioned that PRs really should be merged in 1-2 days. Each time though, the actual time to merge a PR is higher. Sometimes lots higher.
What follows is a personal opinion on what you, and I can do to help lower this. This method is something that I’ve used in the past to great success, and you’re able to pick up as just an individual, this doesn’t require the whole company to try it all at once.
Lets start with a bit of why I think these issues arise.
What’s hard about a PR
For a normally sized PR, there are a few things that are difficult when reviewing
- Context
- Time
- Latency
Context: When work across teams, or even in large teams you're often assigning review to someone who knows the code well, but might not be fully aware of all the work you're doing. To get around this, as a good PR owner, you of course wax lyrical in the PR description about what your PR is, and why it’s doing it. As a reviewer though, it’s a bit of a lottery if that’s enough to go on, or whether it answers the kind of questions that you need answered.
Time: As a reviewer, often you’ll just get hit with a PR in your inbox without much warning. Maybe you knew it was coming, but there’s often cases where you just suddenly have a fresh piece of work on your plate that’s just arrived without so much as a ping on Slack. Now your week just got effectively shorter for you to actually perform any work that you wanted to do.
Latency: This affects both sides of the review. It’s hard to ask questions, and get responses fast. If you ask a question one day, it’s very possible that by the next day the other person on the review has forgotten about them, and will need to nudge them for an answer. Maybe you’re answering any questions asked on your PR in seconds, but your reviewers never seem to get back to you as fast. Perhaps because they’ve expected it to take a while to receive a response, and thus have moved on to the next review. For every question that goes back and forth in a PR you can lose somewhere between minutes, and days of time.
How can you fix that?
With Pair code review.
In my opinion it’s as simple as that for a vast amount of cases. In a perfect world here’s my steps for a quality, and fast code review:
Before you start work on code, you should have an understanding about who you’ll be needing as a reviewer, notify then that you’ll be interested in performing a pair code review. Just to be polite about what you’re expecting from the reviewer.
Now, complete the code (the easy part).
Put up the review. The review quality should be just as good as always. This technique does not allow you to have a lower quality description, or lower quality clarifying comments throughout the PR.
Here's where things take a small turn.
Send a calendar invite to your reviewer(s). When doing so, I follow these rules
- Your PR should be sized that this should take 30m - 1h max. If it’s a larger PR, break it down. No excuses. This is especially important for this technique.
- Your calendar invite should respect the reviewers time, make sure you’ve not just turned their day into a crap meeting-fest.
- You should aggressively give back time. Indicate this on the invite. If the pair-review only takes 15m of the 30m, end the meeting straight away and relish. Nothing is worse than someone stretching a meeting just because it finishes 10 minutes from now.
- When it's time for the start of the meeting, make sure you’ve got a good place to go to. If you’ve invited more than one person, grab a room with a TV, otherwise go to the reviewers desk.
Running the review as a reviewee
Your job is to sit near/next to your reviewer and attempt to help with any and all context. You’re there to answer the "why did you do this" kind of questions. You should be taking notes, for all the changes that are desired.
Often i’ll have the review open on my own laptop, and write down what the reviewer tells me to change as comments on the PR like they’d done it.
Make sure to ask any clarifying questions straight away. Get that latency down to seconds rather that hours or days.
Come prepared with justifications for what you’re doing. It should be rare that you answer a question with ‘i don’t know'. Remember that you’re asking for the gift of your reviewers time, respect that by being prepared.
If a large change is required and/or its clear the review is going to be rejected, clarify how the solution should be rearchitected. Then, stop the meeting, and give back the reviewers their time. Alternatively, if you’d like this time can be used for some short pair programming to make the next session more likely to succeed.
As a reviewer
If possible, it’s good to have spent the 5-10 minutes before the calendar invite familiarising yourself with what’s there. This doesn’t have to be time reviewing, but should help you know if you’re the right person for the job, and have your brain running in the background about things you should check. Often the best time to do this is as close to when you received the invite as possible. This will allow you to respond fast if you're not the right person for the job.
When in the meeting, you don’t have to constantly talk, being silent is expected and totally ok. The reviewee is there to benefit your ability to review, but will be getting a lot of of this. You don't need to feel pressured to make conversation, or constantly update what you're thinking. You should feel free to review as you usually would. The only change is to verbalise your actionable comments, and ask questions straight away rather than on the PR.
Be clear at the end of the meeting what actions you’re expecting from the reviewer, and if they need to either book more time for a second review or, if you’re happy for them to merge once the changes you’ve asked for are applied.
For subsequent reviews, sometimes this whole process isn’t needed. Chances are your views have become so aligned that the number of questions, and thus latency is going to go down quite a lot.
Now all together, Merge, then celebrate.
What you get
This seems like a complex series of steps but overall boils down to
- Prepare your PR
- Review together
In my experience i’ve seen this consistently bring down PRs from up to 1-2 weeks to 2-3 days.
One thing to note is that this may feel like you’re losing time as a reviewer, as now you’ve suddenly got 10 calendar invites this week. My argument against that is that this approach should cut way down on your actual time spent per PR. There’s next to no context switching as questions are fielded back and forth without delay, thereby saving time. Your time spent writing questions and understanding answers is reduced to how quickly you can have a conversation.
As a reviewee, if you’re expecting to start a project that is going to require a lot of reviews from a particular person, do the courteous thing, and check in with them at the start. Make sure they have the bandwidth for this kind of thing. Perhaps you’ll need to help let their team know that you’re going to have to ‘steal’ them for a few hours a week as your project goes on.
Overall, feel free to do this, feel free to not. This is just a personal thing that I’ve found successful in the past, and would be happy to do in the future. This approach doesn’t need buy in from any more than you and your reviewer(s), so feel free to just give it a go one day if you feel that your PRs are taking a little longer than you’d like.