Anatomy of a perfect pull request

What does a good pull request look like?
443 readers like this.
plant growth

Opensource.com

Writing clean code is just one of many factors you should care about when creating a pull request.

Large pull requests cause a big overhead during the code review and can facilitate bugs in the codebase.

That's why you need to care about the pull request itself. It should be short, have a clear title and description, and do only one thing.

Why should you care?

  • A good pull request will be reviewed quickly
  • It reduces bug introduction into codebase
  • It facilitates new developers onboarding
  • It does not block other developers
  • It speeds up the code review process and consequently, product development

The size of the pull request

devloper - size of pull request.png

The first step to identifying problematic pull requests is to look for big diffs.

Several studies show that it is harder to find bugs when reviewing a lot of code.

In addition, large pull requests will block other developers who may be depending on the code.

How can we determine the perfect pull request size?

A study of a Cisco Systems programming team revealed that a review of 200-400 LOC over 60 to 90 minutes should yield 70-90% defect discovery.

With this number in mind, a good pull request should not have more than 250 lines of code changed.

pull_request_size_view_time.png

Image from small business programming.

As shown in the chart above, pull requests with more than 250 lines of changes usually take more than one hour to review.

Break down large pull requests into smaller ones

Feature breakdown is an art. The more you do it, the easier it gets.

What do I mean by feature breakdown?

Feature breakdown is understanding a big feature and breaking it into small pieces that make sense and that can be merged into the codebase piece by piece without breaking anything.

Learning by doing

Let’s say that you need to create a subscribe feature on your app. It's just a form that accepts an email address and saves it.

Without knowing how your app works, I can already break it into eight pull requests:

  • Create a model to save emails
  • Create a route to receive requests
  • Create a controller
  • Create a service to save it in the database (business logic)
  • Create a policy to handle access control
  • Create a subscribe component (frontend)
  • Create a button to call the subscribe component
  • Add the subscribe button in the interface

As you can see, I broke this feature into many parts, most of which can be done simultaneously by different developers.

Single responsibility principle

The single responsibility principle (SRP) is a computer programming principle that states that every module or class should have responsibility for a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class.

Just like classes and modules, pull requests should do only one thing.

Following the SRP reduces the overhead caused by revising a code that attempts to solve several problems.

Before submitting a PR for review, try applying the single responsibility principle. If the code does more than one thing, break it into other pull requests.

Title and description matter

When creating a pull request, you should care about the title and the description.

Imagine that the code reviewer is joining your team today without knowing what is going on. He should be able to understand the changes.

good_title_and_description.png

What a good title and description look like

The image above shows what a good title and description look like.

The title of the pull request should be self-explanatory

The title should make clear what is being changed.

Here are some examples:

Make a useful description

  • Describe what was changed in the pull request
  • Explain why this PR exists
  • Make it clear how it does what it sets out to do— for example, does it change a column in the database? How is this done? What happens to the old data?
  • Use screenshots to demonstrate what has changed.

Recap

Pull request size

The pull request must have a maximum of 250 lines of change.

Feature breakdown

Whenever possible, break pull requests into smaller ones.

Single Responsibility Principle

The pull request should do only one thing.

Title

Create a self-explanatory title that describes what the pull request does.

Description

Detail what was changed, why it was changed, and how it was changed.

This article was originally posted at Medium. Reposted with permission.

User profile image.
Software Engineer - Github

4 Comments

I'd like to see an explanation of what "pull request" means, that hopefully connects to the meaning of "pull" and "request" in their more common uses. Showing examples of pull requests isn't helping me understand this.

In Git (or other dvcs) a "pull request" is a request to pull your changes from one branch to another, usually from a development/feature branch you've been working on to the mainline branch for the project.

As part of that the owners of the project will want to review the change that you are proposing they pull into their codebase. The suggestions here are to help make that review process go quickly and smoothly. As useful as your proposed change may be, a poor pull request can result in your change being rejected.

In reply to by Greg P

Thanks for sharing this wise advice, Hugo. Many people will benefit from this.

I'm not sure I'd equate SRP to a Pull Request. If you break down a Pull Request into smaller pieces, the reviewers may never be able to see the cohesive feature, or how changes in the first pull request may be used in relation to the overall big picture. I'd never approve the pull request that introduces building an wheel, if I later learned it was intended to be used to help build a bridge. Pull Request that are destined for Master branch, should be feature complete, giving the reviewers the clearest picture of why any of the code has been added to the code base.

Creative Commons LicenseThis work is licensed under a Creative Commons Attribution-Share Alike 4.0 International License.