Reviewing pull requests is often really hard. But so is getting your code reviewed. Though there’s a lot of potential value in pull requests, you need to ensure you have the right code review process so you can reap the benefits without being slowed down by inefficiencies and harming your team’s morale. In this post, we’ll walk you through a list of pull request best practices intended to help your team get the best out of this process and make sure that everyone is writing good code.
Table of Contents
- Pull Request Best Practices for the Reviewee
- Pull Request Best Practices for the Reviewer
- General Best Practices
- Standardize Your Pull Request Process
Pull Request Best Practices for the Reviewee
Let’s open by focusing first on the reviewee’s side.
Start the Process Early
Good feedback is early feedback. There’s no need for you to wait until you’re done with your change to submit a pull request. GitHub, GitLab, and Azure DevOps all allow you to create draft pull requests to collect early feedback. (By the way, GitLab calls “pull requests” “merge requests.”)
With draft pull requests, you can start getting feedback on your changes way before merge time. You can then incorporate the suggestions you’ve got when it’s time to submit the actual pull request, improving the quality of your changes and making it more likely for your pull request to be accepted swiftly, speeding up the development process.
Prefer Smaller Requests
This is an unoriginal recommendation, and there’s a reason for that: it’s a tried-and-true piece of wisdom. When it comes to pull request size, keeping them small will make them more likely to be reviewed in a timely manner and in a comprehensive way. You would be surprised just how much this can speed up the development cycle.
Also, small pull requests are most likely to be focused, which brings us to the next point.
Keep Pull Requests Atomic
Keep pull requests atomic. That is, a pull request should refer to a single logical change. In other words, when someone asks about what your pull request does, you shouldn’t need to use the word “and” when replying.
How does this mix with the concept of the Boy Scout Rule (i.e. the idea that you should always strive to leave the code better than you found it)? Let’s say that, while doing your task, you found some opportunities for improvement, such as removing duplication, making some other refactoring, or even adding a new unit test.
Would performing one improvement inside the same pull request make it non-atomic? I don’t think so. In my experience, these types of improvements need to be performed ASAP. Otherwise, you run the risk of turning them into TODO items that will be inside the code for the rest of its life.
Keeping pull requests small and atomic is easier said than done. In the flow of work, it is easy to fall into old habits and end up creating PRs that require time-consuming reviews. It can be hard just to remember to pause before you create a pull request to add a description! LinearB’s WorkerB bot will help you and your team improve by notifying you when a PR is too big.
Add Plenty of Context to the Description
Remember that your reviewer possibly knows nothing about your change. When filling a pull request’s description, try to do it in a way that answers the following questions:
- From a business perspective, why is this change important? How does this move the team/organization closers to its goals? Linking the pull request to the ticket on GitHub/Jira/Azure Devops is an important part of answering that question.
- Does the change contain code that contradicts some best practices or conventions adopted by the team? If there’s a legitimate reason for this exception, you should clearly document it in the description.
- Will people in the future be able to understand this change by reading the pull request’s description? Remember that pull requests are also a form of documentation that can eventually be of invaluable help for other people, or even your future self. Have someone who’s as removed as possible from the issue review your pull request’s description to check that the description is truly easy to understand.
If extremely necessary, short explanations about what the code does are fine. Ideally, the changes you made should be clear from the commit messages inside your PR. Lengthier explanations in prose are not only hard to follow, but also useless, since the diff is right there. As a general rule, make your description about the “why” and not the “how.”
Pull Request Best Practices for the Reviewer
Now, let’s cover the other side: best practices for reviewing pull requests.
Don’t Be Quick to Assume Mistakes
When reviewing a pull request, you shouldn’t act as a prosecutor but rather as a cooperator. Yes, you must protect the quality of the codebase, but you can do that by working together with the person who submitted the change.
So, while reviewing the changes, don’t be quick to denounce what you perceive as mistakes. When you see something that doesn’t quite make sense, there could be a good reason for that. Instead of an accusatory mindset, adopt an attitude of humble curiosity.
When There Are Problems, Judge the Code and Not the Person
Of course, every now and then you will find actual mistakes. When that happens, remember there’s a person on the other side of this process. They’re—most likely—doing their best and the mistake was made in good faith.
So, remember to keep things as impersonal as possible when pointing out problems. “There’s a problem in this line” is better than “You made a mistake in this line.” Additionally, try to phrase your concerns as questions wherever possible, and also provide suggestions or even link to external resources—e.g. documentation or a blog post—when you think that’s helpful.
Refuse the Pull Request If You Don’t Have the Bandwidth
It’s not great when pull requests take a long time to be accepted and merged. When that happens, pull requests become a bottleneck in the software pipeline, engineers become either idle or forced into frequent context switches—which add to the cognitive complexity of developing software—and the likelihood of merge conflicts go up.
That’s why teams should try to reduce their pull request review times. As a reviewer, you can contribute to that by simply refusing to take a pull request if you don’t have the bandwidth. It’s better to be up front about it and let your coworker know your plate’s too full than to say “Yes,” and then take too long to review the pull request.
Someone taking on a bunch of PRs is a sign that they could be overextending themselves and on a path to burnout. As an engineering lead, it can be hard to keep track bandwidth for each of your employees and intervene before they’re burnt out. Thankfully, with LinearB you can do exactly this and make sure that your team members are working at a sustainable pace.
Loop in Someone Else if There’s Need
Sometimes someone assigns you a pull request but you don’t feel you have the knowledge or experience to review it. Maybe you’re qualified to judge the architectural choices, but there are security concerns you think are better off reviewed by someone else.
When that’s the case, don’t waste time getting that person on board. If you loop them as soon as you realize you need them, the likelihood of that critical portion of the changes being comprehensively reviewed in a timely manner goes up.
Request Synchronous Communication When Needed
Some pull requests are harder than others. You’ll often realize that a particular change is generating too much discussion. The back-and-forth isn’t productive, and you feel like asynchronous communication ceased to be the best medium for the exchange.
Well, if that’s the case, reach out for another medium! If you feel the need—and there is the possibility—request synchronous communication. Jump on the text chat or even on a call if that makes sense. I’m all for asynchronous communication, but sometimes the quickest and most productive way to settle an issue is a live conversation.
General Best Practices
Before wrapping up, there are some bonus tips I’d like to share.
Automate as Much as Possible
There are many tools that can automate some of what could go in a pull request review. Things like static analyzers and quality gates can detect many types of problems, including adherence to coding standards, possible bugs, and potential security vulnerabilities. Make good use of the automation at your disposal and keep reviewers for concerns that require human intelligence.
Want to learn how to implement custom dev workflow automation to cut code review time by up to 40%? Watch this hands-on workshop to set up the free gitStream tool to automatically:
Classify and route pull requests Add estimated review time to PRs Apply custom rules
gitStream product lead, Ofer Afias, will take you through a live demo of the three core functions when getting started with gitStream workflow automation to scale your team efficiently and improve your developer experience.
The Main Branch Should Be Sacred
If you’re practicing continuous integration/continuous deployment, then every successful merge of a feature branch into the main branch results in the application being deployed to production. However, even if you aren’t, main should always be in a state where it’s deployable. In other words, you should strive to keep all of the following requirements true:
- At all times, code in main builds and all the tests pass.
- It’s always safe to branch off of main when starting work on a new feature or fix.
- The timeline of main should never be rewritten—i.e. git push –force to main is forbidden.
- All production changes enter main through a pull request.
Standardize Your Pull Request Process
Pull requests offer great opportunities for the exchange of knowledge and experience and for the early identification and correction of issues, besides serving as documentation and enablers of traceability for the organization.
But if not done right, pull requests can derail into sources of misery for the whole team. Hopefully, with the help of the tips you’ve read today, that won’t happen to your organization.
Of course, your mileage may vary, and some of the tips might not work for you. That’s fine. What matters is that you standardize your pull request process to remove as much friction as you can, so the requests can serve as levers taking your engineering team to new heights.
A great way to start standardizing your code review process is by establishing a review checklist. Check out our post on what you need (and don’t need) in your review checklist to get started.
Thanks for reading.
