Pull Requests

14th August 2020 · Matteo Merola

When working in a team, the priority should be to maximize the overall team throughput instead of just focusing on having some individual contributors only going at max speed.

For this reason, I believe that having a very efficient review process can really help the team to be faster in implementing more performant and robust code. In this article I will try to list some of the reason supporting my belief.

Pending code reviews massively slow down the team

If we see a team as an array of threads, having a pending code review is like having one of the threads waiting idle for the network. More precisely, let’s consider one engineer blocked on pull request review by another one. There are two factors to consider:

  • the cost of context switching
  • the cost of memory allocation increase

Even if some engineers are good at context switching, they still have to allocate mental space to keep track of the different threads of execution. This reduces performances of course. Ideally we would like to also avoid context switching as much as we can to reduce overhead.

When considering the worst case scenario instead, an engineer blocked on code review, cannot make any progress at all. Even in the most well oiled organizations this cannot always be avoided and sometimes work amounts to synchronuous obstacles in order to get work done.

If we abstract out from our daily work, if we take a step back and we think about the limiting factors that force us to add threads of execution, in the end it all boils down to waiting for someone to write the code or for someone to review it. Sometimes we are also blocked by the work of other teams or by infrastructure changes that need to happen, but this is not fully in our control if you think about it. In the realm of the team it is really just waiting for someone to write the code or to review it.

If we adopt this principle, there are two sides that we can tackle:

  • reducing the limit of work in progress
  • reducing the limit of work under review

Since writing the code and reviewing is a strongly dependent relationship (one cannot review code that hasn’t been written yet), we cannot reduce the limit of work in progress unless we reduce the limit of review in progress first.

But how can we become better at reviewing? Once again, two sides:

  • reviewer side
  • submitter side

Reviewers

The first tip is to have a simple way to understand what reviews your on the hook for. Having such list helps you to organize your time and be more efficient and proactive with the reviews you should tackle.

Shortening your event loop can also be a good tip to become better at reviewing. When possible, being interrupt-driven greatly helps a colleague not to be blocked a day or two (if your event loop lasts a couple of hours instead of a day, you have time to fit a review in between).

If you need to finish something, a complex piece of code or even just a thought, do put off a code review. If your thoughts last days, maybe they are not good thoughts in the first place. In a sort of way, making a review is like going to the bathroom:

  • it should not take that long
  • you have to do it
  • if you are finishing a thought you can hold it for a while
  • if you like your pants, you should not hold it that much

Sometimes priorities are just off within your team, just in case, here’s how they might resemble:

  1. outages (fix first)
  2. customer service (customer first)
  3. code review
  4. code production

Once again, in the end a feature is done when it’s merged, and it’s merged when it’s reviewed, so that should happen quickly too.

Being crystal clear helps a lot. More than crystal clear, sometime we should also be stupidly clear and I would dare to say to always leave a comment. If everything looks clear to you, write a good “Looks good to me!” or “This is awesome” comment.

How much time should we spend on code review? There’s only one answer to this question: “As long as it takes.”, especially because it’s got to happen anyways. More on the practical side, one thing that I do is to have multiple moments througout a day in which I review code (generally morning, after lunch, before logging off). My goal in doing this is to reach the almighty inbox zero.

Of course you are not a machine, reviewing 4000 lines of code is extremely difficult and will probaly take a long time. Here’s the issue is with the size of the pull request in the first place though.

Being thorough, each and every time is something that should be a given because if a colleague asks you (sometime even specifically) for a review, their goal is to get some useful comments from you, you should respect that. If you are unable to do just that, please report it so that your fellow colleagues know.

Having a review that drags for multiple days with different revisions is not a very efficient thing to strive for. In this case, when you start a review try to finish it as soon as you can (being thorough). If it’s not possible you should report it for it not to happen again in the future.

Submitters

Your colleages are now making reviewing a top priority job in your team, that means that you now have a huge responsibility with their regards. Things you could do when submitting a pull request:

  • carefully reviewing the diff before publishing
  • making sure there’s nothing missing
  • making sure there’s nothing superfluous or unnecessary
  • making sure you have written code that follows the guidelines of your team
  • making sure you
  • not submitting a pull request and then immediately push multiple revisions as you find problems (most tool support draft pull reqeusts for this purpose)
  • writing good title and description
  • keeping it small

Keep review request as small as possible but really be biased to small, digestible review requests. When possible, try to break down your large refactor into smaller, easier to reason about changes, which can be reviewed in sequence (or better still, orthogonally). When your review request gets bigger than about 400 lines of code, ask yourself if it can be compartmentalized. If everyone is efficient at reviewing code as it is published, there’s no advantage to batching small changes together, and there are distinct disadvantages. The most dangerous outcome of a large review request is that reviewers are unable to sustain focus over many lines, and the code isn’t reviewed well or at all.

Write good titles and descriptions so that the reviewers know what it is about before even checking out the code. Do not auto generate the description from the list of commits because the description should actually contain the line of thought you followed for the change. It should actually represent that little something that it’s not understandable from the code, the hidden rationale of the changes you propose. The description could also stress out the specific parts to be reviewed more carefully, or the specific approach followed. What you get in exchange is a more thorough review, and one that’s more prompt and to the point.

Acknowledging comments quickly and, in general, having a tight event loop is very important when your code is out for review. Do your best to acknowledge comments as quickly as you can, and particularly to ask for clarification if you don’t understand the comment or want to push back on it. As soon as a reviewer submits their comments, the action is back on you. If the changes requested are straightforward and small, post a new version quickly, to keep the ball rolling. The more quickly you are able to respond to comments and defects, the more likely the reviewer is to still have them paged in, and the more quickly you’ll arrive at consensus.

Don’t wait for a review or a comment with a reactive approach, but be bold and assume a proactive approach when stagnating. Seniors are often involved in many discussions and generally can lack of time in some periods. A proactive approach would make the overall team throughput much higher!

Select your reviewer, don’t add more than needed, don’t add the whole team! You can use the following guidelines to decide who to ask a review from:

  • git blame: did anybody work on this component before?: that person can be the best candidate to review the new changes then!
  • will anybody work on something tightly related to the changes you are checking in? Get their opinion already!
  • your changes are very much focused on something you know you have a colleague that’s very knowledgeable on? Add the guy/gal!
  • got a new colleage to onboard? Add them to the mix (maybe leave a comment saying that you add them for onboarding especially but that comments are welcome)!
  • none of these guidelines can apply? Check out the current review status and choose somebody who’s got fewer PRs to review!

Sometimes it happens that you are very much unsure about the structure of something you want to introduce and you would like to have some feedback. While having a quick F2F meeting with a colleague can help you understand the way to go, in my experience, nothing beats having a code diff with proper comments explaining the idea behind. Especially in this world where communication happens more and more asynchronuously, black on white (well, white on black) always helps. In such cases, I found very helpful to have Work in progress pull requests with only the overall structure you want to propose and little implementation. When enriched with proper comments to explain the rationale behind your idea, this way of proceeding can give you valid insights as soon as possible without having to fully work out a solution just to find out that you completely overlook something that forces you to rethink the entire approach.

Robo speaking

Both reviewers and submitters have a central place to discuss code changes and this place is the Pull Request (or Merge Request for the purists out there). That is the central place, and that is THE place where things are being decided, discussed, elaborated and approved/declined. It does not matter for what company you will work or what kind of project you will help shape, where you have software1 you have teams. One of the most important aspects of programing is communication and unfortunately the code can only represent a tiny fraction of what is communicated within the team and of what the team wants to communicate to the outside world. Almoost every internal communication is around some code and should happen as close as possible to the code: that’s why we link issues and tickets to pull requests, right? Being good at communicating what some code changes should do and how they could be improved is key to the success of the entire team, not only for having clearer rationale but also to enhance coordination in teams that grow over 3 elements. Robo speaking means:

  • always acknowledging when you are mentioned (you are mentioned for a reason isn’t it?), here’s some example comments you might wanna leave:
    • LGTM
    • Ack
    • Great addition
    • Awesome job
    • Can’t wait to merge these changes!
  • be clear with what you mean when leaving a comment, look out for things like these:
    • is my comment vetoing the approval, or is it for future purposes?
    • is it clear why I left the comment?
    • is it clear what I am proposing?
  • be clear when you ignore a suggestion, look out for things like these:
    • is it clear why I will ignore the suggestion?

Conclusions

In any software project, writing code is just one of the processes and definitely not the only one the team should optimize. Code reviews are an essential part of development too. Coding sits at the bottom of the value chain, where you start with good product management and strategy that leads to clearer goals, that leads to better understanding of what to do that lead to improved code reviews that leads to better and more high quality code that leads to faster coding speed in the long run. Time is a scarce resource and multi-tasking doesn’t always pay off well (especially with the human mind). By optimizing the review process we can all get more focused time and grow faster in terms of personal skills and team knowledge (both personal and regarding the knowledge base inherited by prev. generations of team members/managers/strategies/approaches).

1Any software that generates a lucrative amount of money.