This content originally appeared on Level Up Coding – Medium and was authored by Kevin Masur
Some may argue that pull requests are an inefficient or bad practice, however I find they are still useful in many projects. However, to get all of those benefits you still need to have a good culture around how your teams treat pull requests. Here are the things I find lead to a good culture around pull requests.
Quality is considered important
For developers to take pull requests seriously, quality needs to be considered important within your organization. If getting it done fast is rewarded more than doing quality work, pull requests will seem like a blocker rather than a way to improve your codebase and your developers.
The other important point here is that while you can get away with speed over quality for a while, eventually you will start to slow down making future changes due to the high technical debt you have accrued.
Be humble
If you are having to read through the comments others have left on your code, be humble. They are not trying to insult or belittle you. Everyone makes mistakes, and everyone views things from their own lense of experience. Take their feedback and at least consider it as if you might be the one who’s wrong, rather than just immediately trying to defend why you are right. Sometimes you will be right, sometimes you won’t. Truly consider which is the case before acting on it.
Be kind
If you are leaving comments on someone else’s code, be kind. Try to be direct about the issue, but not mean or insulting. Don’t mock or mention anything about the developer personally. Keep it simple and professional.
Everyone should feel empowered to review
It doesn’t matter if the reviewer is an intern on their first day, or if the code was written by a developer with 30 years of experience. Everyone’s input can be valuable, and everyone's code could be improved.
If something is hard to understand, difficult to read, or seems repetitive, anyone can provide that feedback. They don’t need to know the context to give feedback on how readable your code is. Sometimes, having a lack of context can provide good insight into your code for how hard it is for someone new to read.
And if you have a strong culture around everyone reviewing pull requests, you should never be waiting long to get feedback or approvals.
It’s a great learning experience
Everyone can learn something by reviewing someone else’s code or having their code reviewed. Better practices or patterns, or useful tools and tricks. Reviews shouldn’t just be used to improve the code in the review, it should be a way for both the reviewers and developer to improve their skills.
And once you receive that feedback, you will start writing your future code keeping that feedback in mind going forward. So you will write better code from the start in the future, rather than waiting for the PR to get the same feedback over and over again.
It is a great time to include quality control tools
Linking builds to pull requests that run automation tests and preventing merging without a passing build is a great way to ensure the build is always passing on the trunk.
Code quality tools can also be integrated to prevent pull requests from merging if quality gates are not met. They can also be integrated into the PR to include feedback within the code diff. Tools like SonarQube allow for showing code smells and your code coverage in your diff. This makes spotting code smells and missed code coverage a breeze. Linters, security scanning, and any number of tools could be integrated into your pull requests to ensure issues are fixed at review time.
Teams shouldn’t be silos
Teams should review code from their own team, but they should also review code from other teams. Reviewers can often focus on certain issues and ignore others. By having reviewers from outside your team, you can get a wider range of knowledge and feedback. Otherwise, your code quality will live in an echo chamber.
Fix the small stuff
Yes, it can be a little annoying when you get comments on formatting, naming, or other nitpicky issues. But unless it’s purely a matter of preference, I would encourage you to fix it. Those small comments are often so quick and easy to fix that it’s faster to fix them than argue about them. And if they do make the code a little cleaner, you’d be surprised how quickly the little things can start to pile up in a codebase.
Review it before you open it
Don’t waste people's time on the stuff you could have caught yourself. Before you click the open pull request button, review your code the same way you would someone else’s. I often find little issues or things I forgot to clean up when I give my work one last final review. Don’t end up looking like a chump. Only put up code for review that’s ready.
Tests should get reviewed as well as production code
While I will admit to not putting the same due diligence into automated test code that I do into production code, tests should still be reviewed.
They should be clean and easy to read, the same as the rest of the code. They should cover all of the expected cases, and setup and validate those cases properly. And they should avoid doing things that would make the test brittle.
Keep your changes within scope
Comments in a pull request are fine. But sometimes those comments might be outside of the scope of your changes. When that happens, do your best to open a separate pull request right away to address just the comment.
Conversely, don’t delay making changes that are within the scope of your pull request. Saying you’ll fix it in the next one can lead to things getting missed, and changes being harder to review later when unnecessary changes seem to be happening in other PRs.
Avoid Privileged Reviewers
It can be tempting to only allow certain experts to review code, but if this is done for the whole codebase it can lead to an anti-pattern. Only allowing a small privileged reviewer group leads to a slow turnaround time on reviews. It can also hurt the trust between the developers and the reviewers.
If you have a small number of specific, specialized areas of your codebase that only have a few people who can provide valid feedback, then you might be able to get away with this for just those areas. But avoid wide-ranging rules like review councils or only senior developers can review code mandates. It’s slow, stunts developer growth, and puts the burden of quality onto a small number of people, rather than on everyone.
Keep your changes small and with a single focus
Large PRs are hard and slow to review. And that leads to your changes getting reviewed slowly, or getting an overwhelming amount of feedback. Keep it small and focused and you will have feedback and approvals in no time.
Occasionally things require larger changes. When that’s the case, I often make a feature branch and make smaller PRs into that branch for review. That way, all of the parts were reviewed individually, and then at the end, the whole of the changes can be reviewed together. Just keep your feature branch up to date with the trunk as often as you can, and try to get it merged as quickly as possible. Avoid long-running feature branches.
Know when it’s not good enough
When reviewing be clear on how important certain changes are. Don’t be afraid to block changes that aren’t meeting the quality standards. But make sure the developer knows what’s expected of them to get your approval. Also, while it’s fine to notice something else later, don’t constantly move the goalposts on someone. It can be frustrating to have to deal with 4 or 5 iterations of your comments that could have been provided from the start.
Document your best practices somewhere
This can be a useful doc for new developers to review so they don’t feel blindsided when they open their first pull request. It’s also good for documenting why doing things a certain way can be important.
This should be a living document, that anyone can update and improve over time. I have found that some things I considered a best practice a few years ago have now been replaced by an even better way of doing things. When things change, update and link the doc.
Conclusion
Above are the practices I have found are the best way to get the most out of pull requests in your organization. Are there any you felt that I have missed? Or perhaps anything on the list you disagree with? Or any you wish you saw more of in your organization?
What a good culture around pull requests looks like was originally published in Level Up Coding on Medium, where people are continuing the conversation by highlighting and responding to this story.
This content originally appeared on Level Up Coding – Medium and was authored by Kevin Masur