 |
VOOZH | about |
|
VOOZH | about |
We’re so glad you’re here. You can expect all the best TNS content to arrive Monday through Friday to keep you on top of the news and at the top of your game.
Check your inbox for a confirmation email where you can adjust your preferences and even join additional groups.
Follow TNS on your favorite social media networks.
Become a TNS follower on LinkedIn.
Check out the latest featured and trending stories while you wait for your first TNS newsletter.
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.
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 PReither 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.
Start with the code. Make sure the code is:
The following guidelines help DoorDash manage its own PR creation and feedback.
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.
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:
Kiyasu suggests starting with a short template when getting started with PR templates, “because being concise helps the reviewer which leads to better results.”
“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.
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:
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:
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.
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.
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.
