Reviewing Pull Requests

Reading Time: 12 minutes

This is the second post in a series about asynchronous collaboration. By asynchronous, I mean that people don’t work at the same time. It’s common on distributed teams, especially across time zones. You can see all the posts so far on this series right here.

In the previous post, we talked about asynchronous limitations on code reviews:

Code reviews: we know they make our code betterHow do we do code reviews? There’s less agreement on that.

I love pairing for immediate code review, but it requires synchronous communication. For asynchronous code review, we need another option.

Enter the pull request. In this more prevalent approach to code reviews, a programmer keeps a change set on a separate branch, then submits a request to merge it into the main branch. Another programmer reviews the code in that request prior to merge.

Then I shared what I include in my pull request submissions. In this post, we talk about the other side of the interaction: how I do pull request reviews.

Strap in folks, because you need to hear this, and you’re not going to like it.

Why most pull request reviews suck

I mentioned that my favorite form of code review is pairing, but that pull requests work better for asynchronous communication. Pull request reviews don’t have to be inferior to pairing, but they usually are because of the way reviewers do them.

In pair programming, if a programmer has a suggestion for how to change the code, the interaction goes like this:

Programmer 1: “What if we tried [X]?” 

followed by

Programmer 2: “Hmmm. Interesting. Let’s try it. [*types]”

or

Programmer 1: “May I take the keyboard to demonstrate? [*starts typing] it might look something like…”

We have, in pairing, an expectation is that the pair will participate in implementing any alternative that they suggest. Meanwhile, in pull request reviews, I see a lot of this crap:

Original Developer: [*Implements Code]

Reviewer: “I don’t like [X] about this solution. [*blocks merge]”

or the only slightly less atrocious:

Original Developer: [*Implements Code]

Reviewer: “I don’t like [X] about this solution. Have you tried [*vague description of an alternative]?”

This is not better:

Original Developer: [*Implements Code]

Reviewer: “I don’t like [X] about this solution. We might do instead something like this [*pseudocode, or code that doesn’t compile]. You get the idea.”

And these, by the way, are civil examples. I’ve seen this exact thing done regularly except with insults tacked onto beginning like “No offense, but I cannot imagine why somebody would even think to implement it like [X].”

In fact, the state of pull request quality is so bad that most advice I’ve read on how to do a good review only addresses the idea that you shouldn’t insult someone’s code. To be frank, it’s absurd that we have to explain that to grown adults. The only other advice I regularly see says “try pointing out the things you like about their code!” Yes, do that. But that’s still setting a really low bar. The above examples aren’t mean at all, and they’re still bad. We must demand more from adult professional software engineers than “don’t be mean” and “try being nice.”

Why are the above examples so bad?

They’re bad because the reviewer is failing to participate in the development of sound code. Instead, they are dictating their untested ideas from their ivory tower of reviewership. They are delegating responsibility to the original developer to do both the feasibility test and the implementation of those ideas. They’re recording their idea in such a way that they get credit if it works out, but the original implementer undertakes all the risk that it doesn’t.

Screen Shot 2019-12-14 at 11.27.09 AM.png

Because if these ideas, that they fired from the hip, don’t work? It’s the original developer who lost time chasing them down—not the reviewer. This is disrespectful of the original developer’s time, a waste of the reviewer’s knowledge, and a great way to criticize other people’s work without examining one’s own limitations.

In fact, in most pull request reviews, it is dead obvious to me that the reviewer did not pull down or run the code at all. This is flatly unacceptable.

This means that, if the original developer became unavailable to merge this code, the reviewer would be no more prepared to take over development than they were before they reviewed it. Context transfer has failed.

Here’s the proof that pull request reviews could be extremely effective: they are still sometimes useful, even when they’re this bad. So why do we do them this badly, when there is so much potential for them to change our code for the better?

Stop doing these distant, cursory code reviews. You (yes, you) are better than this.

Instead, reviewers: put your money where your mouth is.

Don’t like something about this branch? Pull that sucker down. Test out an alternative yourself. Fix it and push a commit for the original developer to review. This is how open source contribution works, and it’s how collaboration on your distributed team ought to work, too. Here’s a pull request where I did that. The original reviewer did most of the work, and I submitted a change back to him before final merge.

Screen Shot 2019-12-14 at 10.51.23 AM

Once again, if the reviewer lacks sufficient context to modify the code themselves, the pull request has failed on the goal of context transfer. The original submitter should transfer more context in the pull request description until this is possible. Here’s how they can do that.

I understand that other factors may prevent a reviewer from pushing their own changes. Maybe they don’t have push access. Maybe they don’t want the original developer to feel like they’re taking over their plum project, or doubting their abilities. I get it. In these cases, I still pull down and try out my own suggestions, then provide the changes, code verbatim, to the original developer in my comments.

An Example of That

All examples come from this pull request on a Python script. Context: The author knows Python, but isn’t an expert on the particular libraries used. I have used the libraries, so as the reviewer, I do my best to make that knowledge useful to the original developer.

Screen Shot 2019-12-11 at 3.16.11 PM

See? Code. Copy-pastable code. I know this code works because I tried it in the script and saw it work with my eyeballs. Here’s a slightly more involved example:

Screen Shot 2019-12-11 at 3.14.19 PM

For context: the tqdm library is named for the Arabic word for “progress” (تقدّم, pronounced ‘tiqdam’) . The verbatim change that I am suggesting here propagates to a few places in the code, so I suggest changes on those places as well rather than forcing the original developer to figure that out by themselves.

In fact, there’s one more change we need here:

Screen Shot 2019-12-14 at 11.12.33 AM.png

My review should save time and frustration, not create it.

How far should this go? In one pull request review at a previous job, I needed to:

  1. pull down the code,
  2. notice that something was running slowly,
  3. confirm with the original developer that the current speed was unacceptable,
  4. benchmark the code to determine what’s running slowly,
  5. identify an inefficient query,
  6. try a different query,
  7. benchmark the difference, and
  8. submit the results in a PR comment.

It was on a private repo, so I can’t show it to you, but I want to emphasize how far the reviewer’s commitment to participation in the problem-solving can go. Had I pushed all that work back on the original developer, what would have happened?

  1. They would resent me
  2. I would have assumed (rather than understood) whether performance were important at this part of the app
  3. I would be no more prepared to work on the app that I was before
  4. My skills at benchmarking and my knowledge of queries would be wasted, and the original developer would instead have to do a bunch of Googling

And instead:

  1. The reviewer appreciated my help
  2. I understood the role of performance in the app
  3. I could run the whole app on my machine and make changes to it
  4. I got to cement my knowledge by explaining something to someone else.

Wow, Chelsea. This sounds like a massive demand on the reviewer’s time.

The fact that this takes so much time illustrates precisely why pairing is efficient. There is immediate context transfer between two people working together, so all of this work happens in a condensed amount of time relative to the amount of work being done. I’ve written a post on why pairing works if you’re interested, but folks who argue that pairing costs twice as much for the same code are not factoring in the time it would take to do a marginally useful code review.

But if pull requests are the only option available to your team, a reviewer spending this time is still a better use of resources than having the original developer chase down solutions suggested by lazy, delegating commentary on their pull request.

Other things I try to do in pull request reviews

In addition to pulling down, running, and modifying the code myself, I deploy a few other tactics to make my participation in a code review as useful as possible. Here are examples of some of those, from the same pull request as the screenshots above.

Screen Shot 2019-12-11 at 3.15.58 PM

 1. Where additional documentation would be relevant to the changes I am suggesting, I provide links to that documentation. I do not make the original developer go and chase down these things on their own.

Screen Shot 2019-12-11 at 3.15.44 PM

 2. I spend time asking questions. I want to have full context on this code. Remember: if something prevents the original developer from continuing to develop this code, it becomes my responsibility. I must be prepared to handle that responsibility. And I’m not judging another developer’s choices until I have asked all my questions.

Screen Shot 2019-12-11 at 3.15.31 PM

3. I’m pointing out the hidden gems in the code that might otherwise be overlooked. This particular issue gets a lot of people working with pandas, so I’m pointing it out for the benefit of developers on the project who have a lot of experience, but possibly not specifically with pandas.

In addition, I point out when the resources that the original developer finds about something they used might be worth reading with a grain of salt. This is how a review becomes more useful to the original developer than Googling around:

Screen Shot 2019-12-11 at 3.13.22 PM

4. I judge my reviewing skills on whether my review could be supplanted by:

  • A personal proofread
  • A Google search
  • (God forbid) nothing

I want to make sure my review provides value that other resources don’t.

Screen Shot 2019-12-11 at 3.15.06 PM

5. Just as important as feedback about what should change is feedback about what shouldn’t change.

Screen Shot 2019-12-11 at 3.14.58 PM

 6. Finally, in mentioning conventions, I point out that they’re exactly that—conventions. I’ve talked a little bit about the nature of conventions in the footnote on this post, and I think it’s important to call them out for what they are: cultural agreements that may or may not last.

Conclusion

Pull requests have the potential to compete with pairing in their effectiveness as a code review tool. Usually they can’t compete, though, because we have normalized pull requests in which reviewers fail to undertake a participatory role in building the solution with the original developer. I’m not talking about edge cases where the programming version of Dr. House comes in to consult on thorny cases. I’m talking about the vast majority of code reviews where the reviewer is another developer on the same team as the original developer.

A maximally effective pull request suggests solutions…in code. It points out what’s working and what’s not, and links to documentation where useful. It highlights laudable work by the original developer, and asks questions before making assumptions or judgments. It explains the reasoning behind suggestions, whether that reasoning affects functionality or adheres to convention. In short, it demands the reviewer’s full participation in finishing the solution that the original developer started. And it prepares the reviewer to take responsibility for this code in the event that the original developer were unable to complete it.

If you liked this piece, you might also like:

The remote work series (warning: if this post was too salty for you, don’t read that)

The listening series (another thing not to read without a cup of…tea)

The SICP Series (completely unrelated, but this is my blog and I get to plug my esoteric programming endeavors if I want to)

 

13 comments

  1. How do you decide *when* to review pull requests?

    What I’m really asking is: As a person who’s naturally bad at knowing where do direct my attention, how can I pick the appropriate time to focus on code review rather than writing my own code?

    • Hi Senior!

      Good question—particularly because I think the answer depends on what you’re doing. Whether you’re doing a recreational open source project, a professional role as an OS maintainer, or a programmer on private code, for example. It also depends on the urgency of the pull requests—whether the pull requests contain change sets that need to be merged into the main branch before a given deadline.

      Reviewing the code of others versus writing your own code may represent two different priorities, and how they are ordered depends on the goals for the project and, to a lesser degree, your goals as a programmer. This one, I’d venture, is case-by-case.

      • I agree that the elements you’ve named are important elements to pay attention to: The project’s economic role, the project’s goals, your role on the project/team, the project’s role in your self-development, and the PR’s urgency (also impact) to the project. One challenge is ensuring you correctly understand those elements: When tagged, how do you know that a PR is urgent? If something distracts you, how do you *remember* there is an urgent PR? I think there are interesting workflow-design questions that could bear fruit here.

        When you say this is a case-by-case decision, I’ll agree. And yet, all human decisions are made case-by-case. I do suspect there are some principles we can uncover which would guide us toward more effective and respectful collaboration.

        I suspect the way to discover those principles is case-by-case.
        We could collect multiple scenarios, vary the elements, and imagine how different principles would impact the workflow of engineers with different sorts of brains. Much like Test Driven Development is great for many programmers with ADHD, I think it would be possible to find a PR-review workflow which both augmented a reviewers’ Executive Function and empowered a team to move with higher velocity and confidence.

        Do you think this is a worthwhile line of thought?
        It will take a while for me to get around to, but if I was to try this exercise, would you be interested in the details of what I find? Would you be interested in me sending you a pile of scenarios?

      • I do think it is a worthwhile line of thought, and I would love for you to try the exercise. I’d be very interested to see the scenarios you come up with and the details of your findings!

  2. This is valuable and useful to me. I’m an engineering manager who hasn’t coded in any meaningful way since I last worked in a waterfall shop. (I’ve been at this for a long time.) In those days the developer did whatever s/he did and then it sat in the repo until the QA phase began. Code review was simply not a thing. It would have been perceived as a needless slowing of the process. I welcomed code review when I first worked in shops that did it, because I saw material quality improvements being made through it, preventing bugs from ever reaching QA. Seriously, back in the day you’d need 1 tester for every 2 developers, and good practices like code review (plus agile/lean practices, plus CI/CD, plus unit tests, plus…..) has moved that needle hard left so that now I’ve successfully had one tester for as many as 8-10 developers because initial quality was high enough. Anyway, the whole point to this ramble is that I always felt satisfied when actionable feedback was given in a CR and the engineers were using collaborative and collegial language there. Your post today opened my eyes to more.

    • Absolutely—and it’s a testament to the utility of code reviews that they’re still often effective, even when both (or all) engineers aren’t taking a fully participatory approach.

      And that makes me really excited about their potential as an even more effective tool, not only for improving the quality of the code, but also for nurturing team members’ skills and relationships with each other.

  3. Excellent post. One thing I’d add is that one should tread *very* lightly when proposing alternatives to code that already meets requirements. “I can do it better” comments, which often come down to nothing more than personal preference, are the bane of my existence as a reviewee. They were often no fun when I was the #1 reviewer on a significant OSS project either, which is how I learned not to do that. If code is incorrect or egregiously slow, or if it violates conventions, then fine, but otherwise my experience has been that – even without insults etc. – such comments erode the trust/goodwill that is necessary for an effective review process.

    • Absolutely; code reviews help the most when developers are working with one another in service of the goals of the project. When developers propose changes that are orthogonal to the goals of the project, this can often feel like they’re demanding unnecessary time and effort from the original developer in order to satistfy their personal preferences about how code should look.

      In fact, for ANY kind of feedback (and I’d consider code reviews a type of feedback), I’m a big fan of stating some goals up front. This gives my feedback giver the opportunity to tailor their feedback to my goals—and, to leave things alone if they’re tangential to my goals.

      I wrote about this in more detail in a series about giving and receiving feedback. The following post, called “attracting feedback,” talks about how and why I state goals up front when I want a second opinion:

      https://chelseatroy.com/2018/11/26/leveling-up-skill-14-attracting-feedback/

      We could do the same thing in the README.md or CONTRIBUTING.md files in our open source projects, for example, so that folks have advance notice that, for example, “We care immensely about the legibility and maintainability of this code, Having the most clever/runtime efficient implementation is not a goal of ours. So we won’t merge PRs that sacrifice legibility for runtime efficiency.”

  4. This is exactly why I appreciate GitHub’s addition of a “suggest changes” mechanism which allows the reviewer to provide a patch to a pull request that the reviewee is free to accept or not – basically a mini branch/PR on top of the existing PR. It has the benefit of helping the code and also gives the reviewer credit for writing the change (as far as commit history goes) and this provides good incentives all around. I wish people would use it more.

    I mean it isn’t a complete solution but it’s a lot better than the current standard practice.

    • Wow, that’s a pretty cool feature! I didn’t know you could do that.

      I think that I would still prefer pulling down and making commits on my own machine rather than on Github, because it is important to me to ensure that I can both run and test the code in addition to propose changes to it.

  5. I’ve occasionally done this (pushed code onto someone’s PR branch) but i’ve always worried that it’s a bit rude? one of my colleagues suggested pushing stuff to another branch, which the original author can choose to merge if they like…

    • I understand that other factors may prevent a reviewer from pushing their own changes. Maybe they don’t have push access. Maybe they don’t want the original developer to feel like they’re taking over their plum project, or doubting their abilities. I get it. In these cases, I still pull down and try out my own suggestions, then provide the changes, code verbatim, to the original developer in my comments.

Leave a Reply to "Senior" Engineer Cancel reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.