Peer Review Like a Pro
Gitlab calls them merge requests. GitHub and Azure DevOps call them pull requests. Whatever you choose to call it, peer review can be a source of confrontation and frustration. There is no shortage of posts on the internet of people frustrated by the procedure, the anxiety it gives them, or treading on eggshells around those who don’t take feedback well.
It is natural to get a little too close to our contributions and take comments as personal attacks. It is just as easy to leave a brash comment on a PR, whether we meant it or not. I have also seen numerous “drive-by-reviews” approving huge code contributions without really giving them the consideration they deserve.
With that in mind, I thought I would go over how we at Versent approach peer review to get the best out of the process, providing pointers for both authors and reviewers. Hopefully, it can help you take the pain out of the process for all involved.
Things to look out for when reviewing code
To start with, here are a few things you should be looking out for when reviewing code:
The problems we work on have been discussed and agreed upon by numerous colleagues with users in mind. We need to make sure we deliver their desired outcome. We should ask, as the author intended? Is the way it behaves good for its users?
We want contributions to be maintainable in the long term, long after individuals, the context and nuances of the solution are gone. If the code has well-designed automated tests, this goes a long way to solving this complex problem.
Documentation, comments, and declarative naming conventions
Again, with the intention of improving maintainability, we should check if relevant documentation, code comments and naming conventions have been used to make the solution easier for others to comprehend. Did the developer choose clear names for variables, classes, methods, etc.? Are the comments clear and useful? Did the developer add/update relevant documentation?
Code style and conventions are more important than most of us think. You can quickly become accustomed to an approach, expecting code to be stored in specific folders or represented in particular ways. If the code base is standardised and consistent, it will speed up others’ ability to navigate and comprehend what is going on. Principles like DRY, KISS, YAGNI, etc., are all helpful principles that could make up a broader code style and ethos.
Well-designed code is important for longevity. It requires an understanding of the current and future requirements and a degree of creativity. Therefore, it should follow some principles that will allow for it to prove useful and stand the test of time, for example following SOLID principles. Is the code well-designed and appropriate for your system?
Again, this comes down to maintainability. Complex code could be efficient and terse, but if it cannot be understood by those who come after you, it will be changed, broken or avoided in future. If the code could be made simpler, more people could use and contribute to it.
Approaches when reviewing code
Moving on, here are a few approaches we have taken to improve the review process. It is a list of activities and strategies that we have found lead to better feedback.
Check it out
You should be checking out the branch and using it. While it may help you understand what you are reviewing, it also means we have determined how the feature works.
These are a few indicators to use before your comment to indicate its nature. A FIX blocks the PR from being merged, an Observation (OBS) is politely noticing and suggesting things that could be considered but would not block the merger, and a nitpick (NIT) is a minor detail that sets off your OCD. Something like indentation or a spelling error, etc.
- FIX: “This has to be fixed.”
- OBS: “Have you thought about X?”
- NIT: “I know I’m nitpicking, but…”
Be curious and learn through reviews
Ask questions and learn through a PR. Encourage discussion and ask why someone has implemented something a particular way. Struggling to understand? You could suggest the code be made more readable — always a useful suggestion!
By jumping in and reviewing code you are not familiar with or do not understand, you learn. There is no need for a “Sorry, I’m new.” We should all be asking questions; we cannot know everything, nor should we. While you may not be able to approve the PR, at the end of the day, you will better understand another chunk of the codebase.
Trust that the code contributor is trying their best. This is your chance to educate, not to point out their failings or attack their abilities.
Written communication can come off as brash and aggressive. Face to face reviews give you the chance to have a longer conversation and understand the thinking behind the solution as well as gaps in knowledge.
“Could you create a tech debt card?”
Sometimes, the implications of suggestions can blow out the size of a ticket, or heck, the implementor could just be fed up with the ticket. If you see this happening, be understanding, ask them to create a tech debt ticket and leave a comment — “TODO MA-101 should be…”. It means you can track and schedule the work, as well as track it in the codebase should anyone spot it and start asking questions.
Take your time, take a break!
The longer you review, the more brash and more aggressive your comments can often sound, especially when reviewing several in a row. Take a moment, step away from the machine, and return to it when energised.
When contributing code
You are not going to receive quality feedback if your contributions are lacking. You will end up with lots of curt, almost rude comments if you have not done some basics. Here are a few suggestions.
Take your time and do your own review
Before you prod someone for a review, do your own review and mitigate the feedback you may receive with a code change or comment.
Linters are your friends. They will cut out half the issues raised in PRs. If you notice something come up a few times — create/add a linting rule for it!
Document design decisions
You cannot hold people to expectations without articulating them. Give everyone a fair chance by writing up your expectations. Documenting design decisions and code style, along with code examples, will reduce frustration among engineers.
Personally, I love having this documentation in the codebase, rather than confluence/wikis, as firstly, if you can raise a pull request to propose a change to design decisions, you can have a robust conversation transparently, and it will remain in Github/Gitlab for years to come. Secondly, confluence/wikis are where documentation goes to die, it will become outdated in no time at all, and is not maintained in line with the code.
Complex problems or big issues need more than one ticket or conversation. If you run workshops on epics/problems, you can reach a consensus about a solution. That way, you can create a series of tickets, and everyone will know what and why they are doing something.
The longer the code, the more likely you will have pointless “drive-by” reviews. Smaller contributions result in fewer comments, fewer sticking points, higher-quality code, easier tasks to undertake and better-quality reviews.
Read the Acceptance Criteria (AC)
In fact, read the whole ticket, especially the ACs, to check it makes sense and that you have covered all the ACs before you open a PR.
Before you start a ticket, sub-task and get it approved by another engineer. The advantages of this are:
- You get a sense check on your approach ahead of time, avoiding blind alleys
- You can define a clear approach before you start working
- You get a little backup in a PR
- Is a step in the direction of TDD
Sub-tasking is the first step in Test Driven Development (TDD) — the best practice that we should be working towards.
Keep it simple, stupid — it is a common mistake to over-engineer something. You are inviting either a “drive-by” review or a lot of questions on your PR. The more readable your code, the more maintainable it will be in the codebase over time.
Pair-program on complex problems
If you are taking on a big challenge with a lot of complexity, pair programming shares understanding across the team. If two of you can understand it, there is a good chance the rest of the team will.
Comment your code
Adding comments will help you in the future — if you think you might have to explain yourself in a PR, leave a comment and perhaps even an associated tech debt ticket. With this approach, you’ll have fewer “WTF…” moments, as already explained.
Noticed something that could be improved, or plan on making an improvement down the line? Don’t just drop in a TODO comment, create a ticket and reference it in the TODO comment (eg. TODO MA-123 purify this function). Note: there are plenty of tools in the wild that can help you list all your TODO commentary in your codebase.
Find the right person to review
You will be held responsible for your code contribution, so you should seek out the best person for your code contribution. You’ll regret pressuring the intern for approval when you’ve taken down the product.
There is no silver bullet here, but hopefully, the steps above go some way to addressing frustrations. These are all approaches we have been using, and while things are not perfect, we have seen a huge improvement.
Creating a strong and productive team dynamic is easier when we respect and learn from each other. If you expand your interactions in PRs beyond a list of demands precluding approval to conversations and learning opportunities, things will be better for it.
Code quality improvements will flow naturally. It will no longer be a Sisyphus-like figure pushing quality gateways on individuals in an authoritarian fashion. The team will establish a shared quality benchmark once the benefits are discussed and understood. Only then can a team work together to uplift not only the code quality but also each other through a more educational and collaborative working environment.
The Versent & AWS Great Tech-Spectations report explores how Aussies feel about tech in their everyday lives and how it measures up to expectations. Download the report now for a blueprint on how to meet consumer’s growing demands.