On Writing (Pull Requests) Well
My name is Brad Carter and I’m a software engineer at Axon. I want to say very clearly that I am not the world greatest software engineer. (Careful observers may have noticed this fact being hinted at in the blog title.) However, there some aspects of software engineering that I’m decent with and even a few which I’m pretty good at.
One area where I really excel is writing pull requests. Over the last year or so, I can recall 5 or 6 different occasions where someone went out of their way to tell me how readable, understandable, or easy to review my last pull request was.
In this post, I want to share some tips that y’all can use to upgrade your PR game too.
A Failure to Communicate
Like most other forms of writing, pull requests are a form of communication, and understanding the basic principles of communication is one of the most important things we can do to improve how effectively we communicate with each other.
When we communicate, one person, known as the sender, tries to send a message, representing an idea, to another person, known as the receiver. To understand the message, the receiver has to share some context with the sender. If I try to share a message in English with a non-English speaker, the results will obviously be pretty limited.
Similarly, if I try to explain to a non-technical executive that the reason our service was degraded yesterday was because of a “near perfect storm of stack overflow exceptions induced by our recent service migration coinciding with a bad library update resulting in us getting stuck in dependency hell at the same time,” I might as well be speaking a foreign language too.
Ultimately, the key to communication is empathy.
When writing a pull request, I always start by asking, “Do I understand what I am saying here?” (If I don’t, my reader certainly won’t either.) However, I can’t stop there. I also need to ask if what I’m writing will be understandable to the people I’m going to be asking for review, and not just the staff engineer who wrote 80% of the system either. If I’m going to be asking my whole team for review, then I need to ensure that every member of my team, especially the ones at the margins, has the context needed to understand my message.
Saying the Quiet Part Out Loud
I think that there are two big mistakes people make when writing pull requests, and the first is that they simply don’t include enough context.
A mantra I’ve often heard repeated in software engineering is that “the best code is self documenting,” and I think that this idea is sometimes unconsciously extended to cover pull requests also. Even if we agree with the premise that good code documents itself (which, to be explicit, I do not consider to be a settled question), we’re still only getting half the story.
Under the very best circumstances, source code can only tell us what it is doing, but not why it is doing it. It can only tell us how it is solving a problem, but not what problem is being solved. Even if the code is factored out to functions that respect the single responsibility principle and all our methods and variables have sensible, descriptive names, the line val force = mass * acceleration only tells me that we are calculating force and not why we are doing so.
This an issue for reviewers because solutions exist in the context of problems and I find that I am very rarely able to understand a solution without understanding the problem first.
The way that we solve this problem for our reviewers is by saying the quiet part out loud. Rather than forcing our readers to toil over contextual clues to infer the missing parts, we can save everyone a lot of time and effort and frustration by just giving them the relevant information upfront in the form a pull request description.
What information is relevant? Good question! It ultimately depends on what your readers need (communication is all about empathy after all,) but there are a few pieces of information that I usually include by default:
Background - This is information about where the system or feature that we’re modifying fits into the overall ecosystem of systems and services. For example, Our team owns the Inventory Service which tracks the products we stock in our warehouses and how much inventory we currently have on hand. This allows us to fill customer orders and to know when to order more inventory from suppliers.
Problem Statement and Impact - This is a description of what’s wrong, and if it’s not self-evident, how the problem negatively affects us or our customers. To continue the example, Currently, the Inventory Service represents the product descriptions that are displayed on the store page with ASCII strings. However, we recently expanded our business into south east Asia, and need to be able to support product descriptions in local languages or else risk losing business in these markets.
Solution - This describes how we’re going to solve the problem. Continuing, We can solve this problem by changing the application to represent product descriptions with Unicode strings. We will also need to modify the database in the future to support Unicode descriptions too.
What this PR Does - It’s common for larger features or revisions to be split up among several PRs. To set expectations about what the current PR does, I will roadmap the major (and minor) effects of the current PR. Continuing: This PR modifies the application to use the UTF-32 character encoding instead of UTF-8. I also refactored some code to a function in InventoryEngine.java to remove some duplication.
How We Know the Change Works - I will also usually describe the steps I took to validate my changes. Depending on the change or system, this could be as simple as adding adding a few unit tests or a description of some manual exploration that I did in the user interface. If you’re modifying graphical or user interface components, a screenshot can be worth a thousand words too. Wrapping up the example, To test the changes, I added three tests to InventoryServiceTests.java to verify that we can set product descriptions in Thai, Malay, and Vietnamese. I also manually tested the changes in my development environment and have included screenshots below.
Ultimately this template is just a starting point and it needs to be modified to match the specific needs of your reviewers. However, since I’ve started using it, I have found that providing descriptions in this format has helped me pre-answer most of my reviewers’ factual questions (e.g. “Why did did you change this line of code?”) and lets us quickly move on to the more interesting and important analytic and strategic questions (e.g. “What are the long term effects of solving the problem with strategy X versus strategy Y?”) By using this template, I have found that I usually get feedback (and ultimately approval) on my work faster compared to when I don’t provide a description or other context.
One Thing at a Time
The second mistake people make when writing pull requests is trying tell more than one story at the same time.
Another mantra that I often hear repeated in engineering is the single responsibility principle, which is the idea that each module, object, method, and even variable within a program should have exactly one responsibility or purpose. In contrast with the mantra from the last section , I rarely see this one intentionally being applied to pull requests. This is too bad, because there are a lot of benefits to tightly scoped pull requests. Some of the big ones include:
Reduced defect rates - Tightly scoped pull requests tend to be smaller and smaller pull requests are less likely to break production because they are introducing fewer changes at a time than larger pull requests.
Faster iteration velocity - If, for convenience, I lump three unrelated changes into the same pull request and one of those changes causes a significant discussion or argument during review, the result is a lot of inconvenience for myself and others. All three of my changes end up getting held up. On the other hand, if I separate those changes into three pull requests, then only the controversial one gets held up in debate and I can still make progress with the other two. Additionally, I can also solicit reviews in parallel instead of having to wait for one reviewer who is willing to pull triple duty.
Easier roll backs - In the (less likely) event that I introduce a defect into production, it’s also much easier to roll back a tightly scoped pull request because I don’t have to worry about also rolling back and then later restoring the bystander changes which were unrelated to the bug.
One of the biggest benefits of all though it that it makes life a lot easier for our reviewers, which in turn makes life a lot easier for us also.
Pull requests are narratives. They tell stories about problems and how we intend to solve them, and the easiest stories to understand are the simple ones with a single protagonist and a single villain. You reviewers are busy and they don’t always have time or energy to fully read and appreciate your magnum opus. Sometimes, they may skim it for you as a courtesy, but in my experience, it’s also just as likely to end up collecting dust in their reading pile too.
We frequently need to tell big stories to drive impact and create change, but to do that effectively, we need to break up our epic space operas into small, focused chapters that each tell one part of the story at a time.
If you do this favor for your readers, you will get more reviews, you will get them faster and with less badgering, and the quality of your reviews will go up as well. Ultimately, it’s a win-win situation for everyone.
Inclusive Communication
On a previous team, I once had a version of this conversation with a colleague who after hearing me out responded, (paraphrasing) “Sure, that all sounds well and good, but at the end of the day, I’m very busy and this all sounds like it’s just more work for me. On top of that, the good engineers will certainly still be able to understand my description-less 800 line PR just fine and they’re the ones that I really want reviews from anyways, so why bother with all this extra effort?”
(To this day, I still find it incredible that, in this coworker’s world view, the defining characteristic of a “good” engineer was not the ability to write readable PRs, but merely the ability to tolerate the incomprehensible ones, but that’s not really my point :) .)
Fundamentally, I disagree very strongly with the notion that bad PRs can be used as a tool to weed out bad engineers. The reviewers that difficult PRs select for are merely those with the most experience dealing with difficult PRs (either on the giving or receiving ends). Conversely, the engineers who are most likely to be shut out or left behind by unreadable or inaccessible PRs are those who are new to the team or who otherwise exist at the margins. Specifically, I’m talking about:
your colleague who just joined the team and who doesn’t yet have a good mental inventory of the team’s services and responsibilities.
your colleague who learned (or is still learning) English as a second or third language and who maybe isn’t familiar the jargon or metaphors that you’re using yet.
your colleague with ADHD who struggles with tracking multiple narratives through an 800 line PR. (By the way, this is me. I am your colleague with ADHD :) and this can be really difficult for me.)
At the end of the day, I don’t view writing pull requests well as merely a courtesy or nicety that we use to make our colleagues’ lives easier (though that is certainly a side effect.) Rather, I view writing clear and understandable pull requests as an important tool for building inclusive and welcoming teams. Communicating clearly in our pull requests (as well as in our Slack messages, meetings, and other communications) is one of the most important things we can do to make all members of our teams feel welcome, give them ownership in our charter, and to ultimately empower them to solve the problems that they are uniquely capable of solving.
Wrapping Up
Good job making it to the end of the post!
Today, we talked about some of the basics of communication, discussed a couple of strategies that we can use to improve how we write pull requests, and then looked at the importance of practicing good communication.
Before signing off, I want to reiterate that communicating effectively is not an easy skill. I can tell you from first hand experience that making changes to how we communicate can be a tremendously difficult undertaking, but I can also tell you that it’s worth it.
Communication is a muscle, and it needs to be exercised regularly in order to grow stronger. As you build it up and as you practice empathy and meeting your teammates where they are, I think that you’ll find that you’re saving more time and energy than you’re spending in the long term, and I also suspect that you will find yourself growing closer with your teammates and eventually able to do things together that you weren’t able to do before.
