We’ve just spent the last 3 days crafting our new feature and it’s time to merge it into master so everyone can bask in its glory. But first, the pull request. We’re putting our baby on display for everyone on the team to critique. Beyond our code being technically sound, there are a few things we can do to help our PRs land gracefully.
Generally speaking, it’s all about helping your teammates understand your code quickly and reducing friction. Consider the 2-beer rule":
Your PR should be understandable and critique-able by the average person on your team after they’re 2 beers in.
…or your lead who happens to be up at 1.30am after tending to their infant.
Note “average” – meaning your well-caffeinated principal engineer will be able to glance at it, find all the bugs and maybe suggest a whole new strategy in half an hour. The college hire might be able to explain what’s going on after 2 hours. Consider these pro-tips on how to set your PR up for success.
Smaller is better
Nothing is worse than cracking open someone’s innocuous-looking PR at 4:30pm on a Friday, only to be wacked in the face by 30 changed files. The reviewer may just catch the early bus and wait until Monday.
We do everyone a huge favor by breaking our new feature up into small consumable chunks that are easy to understand. Holding your teammate’s hand over the course of a week may feel tedious, but ends up saving a lot of cycles and down time. Breaking it up might be technically awkward, but that’s okay. Create stubs. Leave plenty of TODO’s. I guarantee that going through this process definitely result in better code and happier teammates.
This is especially important if the author is new to the code base, as they tend to get more feedback as they go through the learning curve.
We’re doing it right when our PRs get in within 4 hours. More than three days and as many iterations starts to get painful.
Optimize the code for readability
Code is written once. It is read many times.
This is where sticking to a style guide is a win. Are variable names self-explanatory and consistent? Or are we now on temp6 but can’t find temp3 or 4? I recommend being a little more verbose when it comes to variables. By adding descriptive names, it helps the reader track the data through different states of the problem.
Linters and other automated code style enforcers are also a valuable tool. No one likes having their code nit picked; authors are embarrassed when someone points out they fat fingered something. People are rarely embarrassed in front of a machine; let us humans focus on more meaningful parts of the code.
Stick with the commonly used syntax when there’s an option. Yes, we could do some gymnastics and take advantage of some obscure syntax to condense our code from 10 lines to 2. But if it takes the new hire half an hour to figure out what the heck is going on in that line, it’s a net loss. Not only is it slowing down our PR, but it’s also costly in 9 months when someone’s trying to fix a bug. Introduce new syntax gradually with simple cases until it reads naturally for the whole team.
Killer descriptions
Frame the problem you’re trying to solve and outline your thought process for the code. You’ll want to err on the side of humility in your tone to ensure you get candid feedback. Perhaps your ideas and your code don’t actually match. You want the readers to catch those things. Also, be concise.
A couple pro tips that help things along:
How can the reader test this for themselves? Are there user flows or tests to run?
Are we touching UI? Screenshots or GIFs save the reader time. Before and after shots are gold.
Are there any un-automated parts of your check in process that you need to run? Simply stating that we performed those eliminates the “just checking to make sure” conversation.
Is there a documented work item that we can link the PR to? This might answer background questions the reader might have, without having to wait for a response.
Manage Scope Creep
Scope creep is a dark and shadowy adversary constantly stalking our pull requests. It’s up to the author to either accept the feedback or gracefully dismiss it such that the code can both improve and get checked in a timely manner. I see a couple common buckets:
NYC – not your code. “Hey I know you didn’t write this but can you clean it up while you’re here?” Careful. Depending on the complexity, this simple refactor runs the risk of causing regressions that there may not be automated tests for. I stick to the simplest, safest of refactors; variable renaming, and code formatting. It’s a definite no if it requires testing unrelated to the initial goal of the PR. If you decide not to integrate the feedback, be sure to acknowledge the legitimacy of the feedback, kindly explain why you’re not integrating it, and file a //TODO.
“Hey, you forgot to / didn’t finish implementing…” This gets back to that earlier idea of keeping our PRs small for easy consumption. Too small, and folks start to give us a hard time about half-baked ideas. Like I said before, leaving a //TODO ahead of time is a good way to quiet that conversation.
“Hey I found an unrelated bug while testing your new feature. Can you fix it?” Sweet, good find! But is the fix immediately apparent, trivial and the super low risk? Fix away! Otherwise, we don’t want to spend too many cycles blocking the task at hand. File the bug yourself and politely reference it in the comment.
The biggest thing we can do to prevent scope creep is to be crisp in the description on our exact goals for the PR. If left open ended, readers will unconsciously tack on their own goals that we likely didn’t implement. We can even pre-empt things with phrases like “I stopped short of because …” or “held off on because…”
Timing
It’s really hard to get folks to pull things off their back burners, so keeping the PR from getting there in the first place is a huge help. I find releasing them to the wild minutes before daily stand-up is the best. Try to avoid times where your reviewers will be away from their desks like before lunch, Friday afternoons or big blocks of meetings.
Maintain a positive tone
This gets at a broader topic of team culture. The author has the most control over the tone in a PR. A positive one can go a long way to get the thing in faster. If you haven’t already, read Michael Lynch’s guide to more human code reviews.
This process is as much about getting our team mates to agree that the code is good as it is about the it actually being good. We’re much more likely to get helpful and usable feedback if the tone remains positive.
I’ve learned the hard way that people are much less likely to participate (and sign off) if they feel shut down or dismissed. This usually happens around a disagreement over some technical or stylistic choice. I’m not saying we should avoid disagreement, but we should disagree in a way that keeps the conversation moving. Things that help:
Acknowledge the legitimacy ideas you don’t agree with
Assume positive intent
Phrase responses as questions, and ask for clarity
Err on over communicating
Emojis and gifs keep things lighthearted
If the team is gelling really well, folks might even take the time to provide examples of what they meant or link you to documentation, saving us hours of googling.
If nothing else, remember that everyone involved in this process shares the goal of checking in high quality code.