TNS
VOXPOP
Will JavaScript type annotations kill TypeScript?
The creators of Svelte and Turbo 8 both dropped TS recently saying that "it's not worth it".
Yes: If JavaScript gets type annotations then there's no reason for TypeScript to exist.
0%
No: TypeScript remains the best language for structuring large enterprise applications.
0%
TBD: The existing user base and its corpensource owner means that TypeScript isn’t likely to reach EOL without a putting up a fight.
0%
I hope they both die. I mean, if you really need strong types in the browser then you could leverage WASM and use a real programming language.
0%
I don’t know and I don’t care.
0%
CI/CD / Software Development

6 Pull Request Tricks You Should Adopt Now: DoorDash

Though more work, 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 bust.
Sep 6th, 2022 3:00am by
Featued image for: 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:

  1. Assess urgency — higher the urgency, the sooner to ask for feedback.
  2. 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.”
  3. Search Google, the codebase, and internal resources.
  4. Try rubber duck debugging.
  5. 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.

Group Created with Sketch.
THE NEW STACK UPDATE A newsletter digest of the week’s most important stories & analyses.