6 Pull Request Tricks You Should Adopt Now: DoorDash
Writing good, clean code is incredibly important but it’s not the only factor as improving Pull Requests can also improve a team’s output and efficiency. The developers at online food ordering service DoorDash adhere to six pull request (PR) best practices that help improve the software development life cycle.
Jenna Kiyasu, DoorDash software engineer, wrote a recent blog post that went into heavy detail on DoorDash’s PR best practices as well as why bad PRs slow down the development cycle.
The Bad Pull Request Snowball Effect
In summary, pull requests are a way to alert other members of a development team to know when you have finished your work on a bit of code that is part of a larger project. “Pull requests happen when you fork/clone a project, make changes to the codebase or add a feature, or do other work, and then are ready to merge your improvements back into the master branch on the main repository,” Michelle Gienow explained in TNS in 2019.
At their worst, “bad PRs can lead to ‘tick the box’ style reviewing in which engineers approve without bothering to read the code,” Kiyasu wrote. Essentially PRs add code to the code base so if a PR either includes code that doesn’t follow best practices, contains bugs that slipped in due to a high volume of code under review, or takes too long to review as the result of a delayed review process because of the PR this affects the code base as a whole.
And in terms of detriment to collaboration, bad PRs are more likely to result in a negligent review than good PRs. The giant snowball here ends up reducing communications across teams as well as skill stagnation for the more junior team members without proper feedback and guidance.
How to Avoid Writing a Bad Pull Request
Start with the code. Make sure the code is:
- Easy to understand.
- Well-structured and consistent with style guidelines.
- Performs the intended functionality. Confirm tests are passing and that they’re passing for the correct reasons.
The following guidelines help DoorDash manage its own PR creation and feedback.
1. Write Descriptive and Consistent Names
Kiyasu says it’s “critical to choose good variable names at the start, saving time and preventing long-term headaches.” Poor variable and function naming make the code harder to read and understand. This is true for the PR reviewer and for anyone else reading the (or adding/ making changes to) code if the PR is approved.
A variable’s name should be self-explanatory, descriptive, and consistent. Poor naming, though it may seem innocuous can also lead to bugs. Once code is merged, replicated, and accepted, it’s incredibly difficult to change any names.
Kiyasu also stresses the importance of paying attention to the larger context of how things are named.
Same name, different meaning.
2. Create a Clean PR Title and Description
The PR context is essential for understanding and reviewing the PR. It’s impossible to assess if the problem is resolved before understanding the fundamental problem itself. The best way to do this is by providing a descriptive PR title and high-level summary at the top of the PR. An example of a high-level summary is something as short as “The feature flag is always turned off.” This points the reviewer in the direction of the feature flag.
DoorDash also suggests the use of PR templates such as:
- Problem
- Solution
- Impact
- Testing plan
Kiyasu suggests starting with a short template when getting started with PR templates, “because being concise helps the reviewer which leads to better results.”
3. Keep PRs Short
“Long PRs yield long review times and longer review times yield longer PRs because engineers try to get more code reviewed per PR,” says Kiyasu. In addition to quicker review times, short PRs also lead to fewer bugs and more comments per line which fosters feedback and collaboration.
The challenge with shorter PRs lies in the discipline as short PRs tend to get longer once edge cases are fixed and tests are passed. How long is too long is also a subjective question that depends on the company’s or project’s best practices. Some teams set numeric guidelines (i.e. under 400 lines of code or under 10 files) while others break them down into logical units like every publisher, controller, or database layer.
4. Manage Disagreements Through Direct Communication
There are instances where direct communication will better serve the review process. When there is more than one engineer whose approval is needed and they are disagreeing or in the event that clarification is needed and direct contact is best, it can definitely be helpful. Some huge positives for direct communication are:
- Feedback may be faster and provide a more thorough review when needed.
- There may be fewer misunderstandings (especially during a disagreement).
- Direct communication allows for prioritization and monotasking during resolution.
5. Avoid Rewrites by Getting Feedback Early
It’s a common question engineers face: go it alone vs. ask for help. Some questions are better to research but there are those that come with a “heavy price to pay” for going in the wrong direction for too long. Kiyasu suggests that “early feedback tends to be more useful.”
DoorDash provided a guide to help determine the right time to ask for help:
- Assess urgency — higher the urgency, the sooner to ask for feedback.
- Make sure to research the correct question — searching “how to plural acronyms work” will likely yield better results than “what’s the plural of PR.”
- Search Google, the codebase, and internal resources.
- Try rubber duck debugging.
- Ask for help.
Creating a PR in “draft mode” is another way to let other engineers know you may need help. A draft PR won’t get merged but a PR for a code skeleton or slightly buggy code may solicit helpful feedback.
The earlier the feedback, the earlier the course correction, the more time saved.
6. Request Additional Reviewers to Create Dialogue
Quick merges don’t always equate to better code merges. One step towards combating that is to request more reviewers to review the PR. Kiyasu says, “If there’s a more general coding style/design disagreement in the PR, others have a chance to weigh in or at least become aware of it.”
Having more engineers look at a PR can add more perspective to the merge and offer more feedback to junior engineers.
In Conclusion
PRs will foster feedback, resolve disagreements, and maintain robust review culture in addition to getting code reviewed. Building the habit of writing clean PRs will pay off in the long run and ensure good practices are being followed even when times are a bust.
