Pull Request Best Practices: Tips for Reviewing and Merging Code

Pull requests are a powerful tool for managing new code releases and code changes within your codebase.  An effective pull request practice not only ensures that developers are making code commits in a non-disruptive manner, it provides a basic review of the changes being made.  This process helps ensure that changes are properly tested and ready for integration into the project.  It also allows for peer feedback and improvement in situations where needed, which improves the quality and consistency of your codebase.

We Are Looking for Code Correctness and Comprehension

The practice of code review and pull requests (PRs) provides a mechanism for reviewing a peer’s work for potential refactoring or potential issues before they happen.  With a good PR process in place, you can limit the number of bugs that make it to production – remember, I said limit.  This is not a fool-proof process to stop all bugs from occurring but rather a (human) peer-based review system.

A pull request or code review is often the first test of whether a given change or contribution is understandable to a broader audience (other engineers on your team).  In many respects, any questions that you get now will be multiplied over time, so each question of comprehension or reliability should be treated as valid and important.

Intended (and Largely Realized) Benefit of Our PR Process

MercuryWorks’ culture, like that of a lot of software companies, is based on giving engineers wide latitude in how they do their jobs.  This is in recognition of the fact that onerous processes tend not to work for teams that need to move fast to deliver for clients – experience shows that bureaucratic rules tend not to work well with creative professionals.

While the practice of PRs for all code commits does have a cost and effect on engineering velocity (it literally slows the introduction of new code into the code base), we have found over many years that a well-operated code review process (and culture of taking it seriously) provides the following benefits:

• Improves code correctness
• Improves code comprehensibility by other engineers
• Promotes consistency across the code base
• Psychologically promotes team ownership
• Improves knowledge sharing
• Provides a history record of the code change and rationale

Remembering That It’s Not Your Code

This will come off as a controversial statement but if you think about it, it’s not: the code you commit is not yours – it’s the team’s.  The team is going to own it, ship it and support it over (hopefully) several years.  Code review has the important cultural benefit of reinforcing this fact to engineers, to let others have input on this collective asset and to compromise for the team’s greater good.

Keep the Right Attitude

It’s human nature to be proud of one’s craft and be reluctant to both open up one’s code to criticism AND seriously consider ways your work could be lacking.  As such, both submitters and reviewers need to have the mentality that PR comments are challenges but are done in a neutral and constructive manner.  It is our intent that all engineers expect to be challenged when submitting a PR and come to value the advice and questions that are part of the process.

Code Author Approaches Are Respected

Authors should generally be given deference to their particular approach, whether it be in the design, or the function of the change introduced to the code base.  A reviewer shouldn’t propose alternatives because of personal opinion – they can propose alternatives if they improve comprehension or functionality but purely opinionated changes to an author’s working approach are frowned upon.

Make Sure Knowledge Sharing Is Part of the Process

The review process allows reviewers to impart domain knowledge to the author, allowing reviewers to offer suggestions, new techniques or advice to the author.  Given the amount of time engineers spend in PRs/code review, the knowledge accrued can be quite significant if done with purpose.  The code review process provides one of the primary ways that software engineers interact with one another, exchange information about coding techniques and receive informal mentoring.

Practices We Value

Small Code Change Size

When it comes to the size of a given code change (i.e. the number of files affected, lines modified, etc.) we recommend that engineers favor small changes.  Small changes are easier to review and also less likely to introduce bugs.  At MercuryWorks we empower reviewers to reject massive submissions and fully-formed modules as too large for a single review.  “Small” is subjective but our general limit is around 200 lines of code per PR.

Write Good Change Requests

The description that accompanies a pull request is the historical record for the code change and is thus not a trivial thing.  The first line of a PR should be a summary of

Be Polite and Professional

During PRs it is critically important to keep all feedback and criticism firmly in the professional realm.  The PR is no place for a keyboard commando or telephone tough guy.  Authors should be receptive to questions/comments on their approach and be ready to explain why they did things a certain way.  Reviewers should be careful about jumping to conclusions on an author’s approach; it’s better to ask questions on why something was done the way it was rather than assuming the approach is patently wrong.

Be Prompt

Reviewers should be prompt with their feedback.  At Mercury, we expect feedback from a code review within 24 (working) hours.  If a review cannot be completed in that time, it’s good practice to respond to that effect.

Respond Comprehensively

Reviewers should avoid responding to the code review in piecemeal fashion.  Few things annoy an author more than getting feedback from a review, addressing it and then continuing to get unrelated feedback during the PR.

Small Code Change Size

When it comes to the size of a given code change (i.e. the number of files affected, lines modified, etc.) we recommend that engineers favor small changes.  Small changes are easier to review and also less likely to introduce bugs.  At MercuryWorks we empower reviewers to reject massive submissions and fully-formed modules as too large for a single review.  “Small” is subjective but our general limit is around 200 lines of code per PR.

Write Good Change Requests

The description that accompanies a pull request is the historical record for the code change and is thus not a trivial thing.  The first line of a PR should be a summary of the changes in the commit and their purpose.

Be Polite and Professional

During PRs it is critically important to keep all feedback and criticism firmly in the professional realm.  The PR is no place for a “keyboard commando”.  Authors should be receptive to questions/comments on their approach and be ready to explain why they did things a certain way.  Reviewers should be careful about jumping to conclusions on an author’s approach; it’s better to ask questions on why something was done the way it was rather than assuming the approach is patently wrong.

Be Prompt

Reviewers should be prompt with their feedback.  At Mercury, we expect feedback from a code review within 24 (working) hours.  If a review cannot be completed in that time, it’s good practice to respond to that effect.

Respond Comprehensively

Reviewers should avoid responding to the code review in piecemeal fashion.  Few things annoy an author more than getting feedback from a review, addressing it and then continuing to get unrelated feedback during the PR.

Conclusion

Pull requests are a powerful tool for managing new code releases and code changes within a codebase. A solid PR process provides a mechanism for reviewing a peer’s work for potential refactoring or potential issues before they happen. With a good PR process in place, the number of bugs admitted into production can be limited.

With a strong PR process you can promote team ownership, improves code correctness, comprehensibility, consistency across the codebase, knowledge sharing, and provide a history record of the code change and rationale. Contact us if you’d like help setting up a tested PR process.