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 - which my team currently uses - 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 what ever 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's make this easy for them.
Keep the title short, but expressive enough to signal what the reviewer can expect. Let's say you're working on an accounting software and there is a new business requirement on how to calculate tax for Switzerland. A title like
Changes for Swiss tax calculation is concise. A title like
Change for tax calculation in Switzerland for new regulation is still concise, with more context. However, a title like
Adding new tax parameters or
Change tax logic are overly generic and won't 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
TaxInternals structure update and CalculateEffectiveRate changes in case of Swiss country code or
Change tax 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
Change Swiss tax calculation for new regulation over
changed Swiss tax calculation for new regulation or
Ability to calculate Swiss tax 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.
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.
Pull request guides and best practices for your team
My team came up with code review request best practices that we circulated, reviewed and committed to and now use it to onboard new members. This was a good exercise to decide what things we cared about, as a team, and to turn best practices into our practices.
One thing that worked especially well was having a list of a few, internal examples of code review requests that were deemed easy to review. Good code review requests come in different shapes and sizes. Having best practices helps set team-level expectations. Good examples help remind people that good ones are not a one size fits all.
Featured Pragmatic Engineer Jobs
- Full-Stack Engineer at Farmlend. £85-95K + equity. London.
- Senior Backend Engineer at Farmlend. £85-95K + equity. London.
- Senior Full Stack Engineer at Perfect Venue. $150-180K + equity. San Francsico or Remote (US).
- Senior Software Engineer at AI Build. London or Remote (Europe).
- Senior DevOps Engineerr at AI Build. Remote (US).
- Full-Stack Engineer at Vital. $70-120K + equity. Remote (Global, within 5 hours of GMT).
- Backend Engineer at Vital. $70-120K + equity. Remote (Global, within 5 hours of GMT).
- Technical Lead - Platform at Vannevar Labs. Remote (US).
- Senior Software Engineer, Fullstack at Vannevar Labs. Remote (US).
- Computational Geometry Engineer at AI Build. London or Remote (Europe).
- Senior QA Engineer at AI Build. London or Remote (Europe).
- Lead Backend Developer at Cineville. €53-79K + equity. Amsterdam.
- Senior Software Engineer, Distributed Systems at Mixpanel. $200-270K + equity. New York, San Franciso, Seattle or Remote (US).
- Senior Software Engineer, Fullstack at Mixpanel. $200-270K + equity. New York, San Franciso, Seattle or Remote (US).
- Principal Engineer at Shoplift. $185-205K. New York.
- Senior Engineer at Sixfold AI. New York.
The above jobs score at least 10/12 on The Pragmatic Engineer Test. Browse more senior engineer and engineering leadership roles with great engineering cultures, or add your own on The Pragmatic Engineer Job board and apply to join The Pragmatic Engineer Talent Collective.
Want to get interesting opportunities from vetted tech companies? Sign up to The Pragmatic Engineer Talent Collective and get sent great opportunities - similar to the ones below without any obligation. You can be public or anonymous, and I’ll be curating the list of companies and people.
Are you hiring senior+ engineers or engineering managers? Apply to join The Pragmatic Engineer Talent Collective to contact world-class senior and above engineers and engineering managers/directors. Get vetted drops twice a month, from software engineers - full-stack, backend, mobile, frontend, data, ML - and managers currently working at Big Tech, high-growth startups, and places with strong engineering cultures. Apply here.