Development

Pull request best practices

When an engineer proposes code changes to the codebase as part of their work and have code reviews in place, they create a pull request. This change consists of a description of the change and the actual changed files. For non-git based workflows, there are other names for this: diffs for Arcanist based workflows or just simply code reviews for teams using Microsoft’s TFS.  The easier it is for other team members to review this change, the better.

Easy to review means easy to understand. Easy to understand results less time spent by the reviewer trying to get their head around the change. It helps the person reviewing to give relevant feedback, having few misunderstandings on the intent of the change. This post collects best practices on creating easy to understand pull requests, or whatever the name for these might be in your work environment.

Clear and concise title

Code reviewers will skim the title of the change to decide if they want to review the code and if so, when. So, let us make this easy for them.

Keep the title short, but expressive enough to signal what the reviewer can expect. Let us say you’re working on a commodity trading software and there is a new business requirement on how to make a reservation on a Purchase/Sales contract tax for Switzerland. A title like Changes for Swiss purchase and sales ctr calculation is concise. A title like Change for reservation calculation in Switzerland for new purchase and sales ctr is still concise, with more context. However, a title like Adding new contract parameters or Change reservation logic are overly generic and will not help reviewers give an idea of the type of change. This will make it difficult for them to decide if they should review the change. What about titles like CtrInternals structure update and CalculateEffectiveRate changes in case of Swiss country code or Change ctr calculation for Swiss businesses in the TX bracket effective from 2019 following new regulation? These are on the other end of the spectrum: they are verbose and feel they provide like too much detail in the title. What is too little and what is too much information will depend on your environment: try to strike the right balance.

As the team grows, it can make sense to agree on conventions to make titles easy to compare. One practice, for example, that my team have adopted is starting the title with uppercase and the first word being a verb in present tense. So, we prefer Changes for Swiss purchase and sales ctr calculation over changed Swiss ctr calculation for new regulation or Ability to calculate Swiss ctr reservation for new regulation. This kind of suggestion can be a nice touch for consistency across code reviews or onboarding new people – but all of it will be specific to your team or company.

The “why” of the change

People reviewing the code spend a lot less time thinking about the change as the author. Providing the context in the description of the change request helps them up to speed on the reasoning behind the code changes.

Having a clear “why” section will make the code easier to review for people for everyone. For people with plenty of domain knowledge, it serves as a quick validation and they will verify if you follow what you set out to. For people with less domain knowledge – such as recent team members or people not familiar with the codebase – it will allow to do a decent review. Having people who are not that familiar with the codebase can both bring in a new perspective and it is also an efficient way to share knowledge.

For teams using project management tools, on top of a short and clear description, linking to the tracking task is helpful. The task often has a lot more context, often conversations and can help the curious reviewer dig deeper and catch up on context. The same goes for linking other resources that can help understand the “why” deeper, such as design documents.

Visual support

For client-side changes, such as those on the web or mobile, few things make a review easier than attaching “before” and “after” screenshots – gifs or video recordings for changes on animations.

This is a practice that not only helps the reviewers greatly, but also the author, who does another verification when creating the pull request. For changes impacting the UI, without this visual aid, thorough reviewers would have to grab the changes, build the product, and run it in their local environment to visualize the changes.

About Codacity Informatica Group  

We are at our heart, engineers who love change and delivery. That what makes us stand out. We recognize that systems must always start and end with clean code. CIG can power your software development transformation from ideation to execution and benefit delivery. We have more than 18 years of experience driving business change in many settings. We are ready to enable you today.