In the quest to promote a culture and ethos of excellence and quality within the dev team at The Curve, it goes without saying that we should have a set of coding standards and best practices under which we operate. Not process for process sake, but a set of guidelines where everyone knows the expected standards to build well-coded software.
In this article we zoom in on Pull requests. Using pull requests to accept changes into a codebase has become a reasonably well-understood concept, to the point where it is broadly considered a best practice.
So how do you adopt pull requests and code reviews into your working process? Surely you tick the box that requires pull request submissions to your repository, and you’re done; you’re now enforcing best practice?
Well…, like most things, it’s not quite that simple. Just because you use pull requests it doesn’t mean you have a good process or culture for peer review:
Practices like the one pictured above can make the notion of pull requests and code review a rather shallow and superficial ceremony for a development team.
So what should a good code review look like, and what are the pain points to avoid? Let’s take a look!
Disclaimer: Before we dig too much into the details, whilst we reference pull requests throughout this post, this post is more about broader practices around code review rather than the specifics of pull requests. After all, pull requests are just the mechanics of seeking feedback. Whether you use pull requests, paired programming or some other mechanics, their effectiveness all boil down to the quality of the code review culture.
Avoid Premature Merging & Critique Your Reviewer
We’ve all potentially been there as a reviewer, we start to comment on a pull request, but before we can complete our review, the pull request gets an approval and is quickly merged.
Let’s face it, it’s frustrating when this happens. Unfortunately, with CI/CD tools often featuring auto-merge functionality, this can be an increasingly common occurrence.
A code review process should be a two two-way exchange:
- As an author: I need to make sure that I get the best feedback.
- As a reviewer: I need to make sure that I give the best feedback.
As the author of a pull request, just because you’ve received an approval doesn’t guarantee your work is free of mistakes or any need for further improvements. An author should mentally critique the feedback they’ve received to check they’ve got the best feedback possible.
For example, Robert might approve my pull request straight away, but if Luke were the original author of the code I’m changing, ideally, I’d hold out for feedback from Luke too. After all, those more familiar with areas of a system are more likely to spot potential issues or idiosyncrasies.
Similarly, if your pull request is approved by a team member who is new to the codebase, they might be able to check your pull request for code style compliance. But, it is unlikely that they’ll be able to comment as effectively on the design or functional accuracy as someone else who is more familiar with the codebase.
Not Too Big and Not Too Small
Pull requests that are too big can be difficult to review effectively. This is because changes like these are often broad, and as the reviewer, it’s easy to get fatigued by a large code review.
In addition, because large pull requests can often take a considerable time to write and review, they can be prone to merge conflicts. This is because whilst the author has been busy working on these changes, the rest of the team has been making changes throughout the codebase, which could conflict.
Feedback is often proportional to the size of the change. By the very nature of being a larger pull request, any remedial work could be proportionately large and take more time to implement, which again presents more opportunities for merge conflicts. Merging large changes can often be delayed, especially if the team feels the change is risky and could impact the team’s ability to ship features or meet other objectives.
On the other hand, smaller pull requests are often easier to understand and carry less risk by virtue of being a smaller change.
However, suppose a change is implemented as lots of tiny pull requests. In that case, it can be difficult for a review to fully understand the context of a change as it becomes fragmented across several very small pull requests.
The goal is to break down pull requests to avoid the pain points typically associated with being either too big or small.
Just One More Commit
When an author raises a pull request but keeps pushing more and more commits to the branch after the PR has been raised, this can often be a warning sign that the author hasn’t been working from a clear plan, and the work could be lacking some design. Of course, this isn’t a hard and fast rule, but anecdotally from our own experience, we find this to be the case more often than not.
Depending on how your team branches work and the size of work items/features, a pull request that generally has a large number of commits (even if these are all authored before the PR is raised) can also often be a warning sign of a lack of design and/or planning.
Refactor Or Behaviour Change, but never both
Making changes to code and reviewing code changes can be made easier when refactoring isn’t intertwined with behaviour changes.
As Erik points out here, this can be applied at a PR level and a commit level. At what level you choose to do this will depend on your circumstances and other working practices within your team.
In the wider tweet thread, Erik also points out the typical impact of changes to tests. Refactoring shouldn’t change the outcomes of tests, but behaviour changes will. And by splitting behaviour change and refactoring into different commits, this makes it easier for a reviewer to spot any mistakes.
Avoid Toil. Use Linters and Static Analysis Tools
Using pull requests to spot code style issues isn’t a sensible use of time. Instead, agree on a coding standard and use linters and static analysis tools to define what is and isn’t acceptable.
Using CI processes (e.g. Jenkins, GitHub Actions, etc.) to enforce code style can cut down on toil in pull requests and help focus the discussions around more meaningful areas such as design, maintainability and functional correctness.
Addressing Feedback as Follow-ups
Sometimes, feedback on a pull request might not be significant enough to prevent the work from being approved and merged, and addressing feedback in a follow-up pull request is entirely acceptable.
If this happens repeatedly, this could be a warning sign of one or more underlying problems, for example:
- The team could be rushing work due to unrealistic deadlines, so work is often put up for review prematurely.
- Not enough time or emphasis is given to planning the work. Or there is a lack of agreement and discussion around how the work should be tackled before implementation.
- The team might benefit from establishing a checklist to prevent minor bits of follow-on feedback from being a regular occurrence.
Encourage Dialogue in Comments
How feedback is delivered can greatly impact how it is received and how likely it is to be acted upon.
Deliver feedback that encourages dialogue and conversation and avoids the reviewer feeling the need to defend themselves. After all, no one writes bad code on purpose.
- It’s not “you”; it’s the code
Remember that you’re critiquing the code, not the person – refer to the code and avoid using “you”.
Bad: You’re being inefficient with the database calls
Good: I’m concerned that this method is making a lot of similar database calls - Questions over statements
When delivering feedback, you can tell someone what to do, or you can ask questions. Asking questions is more open, and by having a conversation, you’re more likely to appreciate why the author opted down the route they took in the first place.
Bad: You’re being inefficient with the database access
Good: This method makes a lot of calls to the database, can this method be refactored to reduce the number of queries? - Leave actionable feedback
No one likes receiving feedback that isn’t actionable and/or doesn’t deliver a benefit. Offer suggestions and encourage discussions.
Bad: This is overly complicated
Good: This constructor takes a lot of arguments. Could the usability be improved by refactoring or using a factory?
Avoid the Rabbit Hole of Pull Request Comments
A large number of comments on a pull request can often be an indication of a lack of planning or agreement around how a feature will be implemented.
When a team ends up in this situation, it is often worth taking a step back and moving the conversation outside of a pull request. More often than not, these situations happen when the team hasn’t had enough time to discuss and plan the work between themselves.
The quickest way to resolve a problem like this can often be to get everyone concerned in a room together or on a call and talk out what needs to happen face-to-face. But unfortunately, asynchronous communication on pull requests isn’t always the best medium for progressing discussions.
Where discussions happen outside of pull requests, it can often be helpful to summarise any outcomes on the pull requests so that anyone reviewing the work is aware of the broader discussions.
There is More Than One Solution
There is usually more than one solution to a problem. Just because the author puts together a solution that doesn’t reflect how you would implement it, it doesn’t mean that their solution is wrong by default. Debating personal preference over another is a good way to increase the amount of toil and friction within a team.
Try to focus on the properties or principles behind the solution rather than the specific implementation. Acceptable solutions inhabit the same properties more often than not, and assessing a solution based on its properties and principles can be a good way of putting specific opinions and personal preferences to one side.
Get Feedback Early
A pull request doesn’t just have to be used when a feature is fully complete. Pull requests can be a good way of sharing your work at any point during the implementation process.
For instance, you may opt to raise a pull request part way through implementing a feature to get feedback from the rest of the team on the approach you’ve taken so far.
Most tools can let you raise draft pull requests to receive comments on in-complete/in-progress work. Even if you don’t have this luxury, submitting a pull request to receive feedback before closing the PR to continue work can be a good way to collect feedback.
As with most things, this approach should be used with caution and shouldn’t be a substitute for planning out work ahead of implementation.
Acknowledge Good Work
Too much of code review feedback can be focused on spotting issues and problems. Don’t forget to call out examples of good work (design, maintainability, best practice etc).
Most of us enjoy receiving praise for a job well done, and after all, code reviews are about feedback – good and bad. Promoting a positive work environment by praising and showcasing good code can help generate a “feel good” environment amongst the team and potentially increase efficiency.
The Curve shares feedback it receives about its developer’s work and shares it with the wider team as a way of promoting a positive environment and giving the praise employees deserve when they’ve excelled on a project. See the example below taken from our internal Slack channel.
Review Your Own PR
Plenty of mistakes can often be caught by reviewing your own work carefully before creating a pull request.
Over the years we’ve had colleagues who used different approaches:
- Some will grab a coffee after they complete their commits but before pushing. By stepping away from the computer for a few minutes, they find they can look over their work with fresher eyes.
- Some will use a diff tool or pull request preview to review the changes. Similarly to the above point, they’re trying to avoid being blind to feedback by being overly familiar with their own code. Often the code looks different enough through these tools to how it looks through their IDE that it helps them spot mistakes, or files that they might have forgotten to commit.
These are the two most common techniques we’ve come across. The key thing here is to find a method that works for you and your team, as what may work for us here at The Curve may not work for you.
Conclusion
Pull Requests are just a tool. Like any tool, pull requests can be easily used as a blunt instrument. Developing a healthy code review culture requires more conscious thought. It needs a team to be aligned on the principles and approaches behind a code review and have a shared understanding of what behaviours positively and negatively impact a code review process and the people involved.
At The Curve we use pull requests as not only are they a valuable tool in ensuring a high level of quality code, but they also help us bring our ethos into action. Shaping pull requests to match our culture and bring out the best in employees is key in creating an environment where employees have the confidence to produce the highest quality work.
Highlighting the positive aspect of one’s work can go a long way in increasing confidence, and promoting an environment where employees have the self-belief to work more individually, creatively and with the confidence of knowing they can experiment with their work to produce the best quality results.
In summary:
- Avoid premature merging – create time to receive feedback.
- Use CI and static analysis to avoid toil – developers expect linters to be pedantic about whitespace, pedantic comments from reviewers feels more frustrating.
- Avoid too large or too small pull requests – size changes sensibly so the reviewer has enough context but isn’t overwhelmed.
- Plan your work – continuing to push changes to an open PR can be a red flag that the work may not have had the right amount of planning or design.
- Structure commits sensibly – help your reviewer and future developers – where possible split refactoring and behaviour changes into separate commits.
- Follow-up versus Fix – sensibly assess if feedback should be addressed now or later. Teams or individuals that always defer feedback could be a warning sign of other issues.
- Leave open, kind and actionable comments – be conscientious about feedback – avoid “you”, ask questions and leave necessary and actionable feedback.
- Avoid the PR comment rabbit hole – no one enjoys a rabbit hole of comments – take discussions outside of the pull request tooling where it makes sense.
- Feedback early – pull requests can be used to get early feedback on how a solution is shaping up.
- More than one solution – be mindful that there is usually more than one acceptable solution, avoid toil and disagreements over personal preferences.
- Review your own PR – it’s easy to be blinded to basic mistakes; take a step back and review your own work before asking a colleague.
- Acknowledge good work – sometimes too much emphasis is spent on flagging issues, don’t forget to praise good work.