Thoughts on getting better at peer reviews

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:

Functionality

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?

Tests

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?

Style

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.

Design

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?

Complexity

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.

Use Fix/Obs/Nit

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.

Be courteous

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.

In-person reviews

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.

Use linters

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.

Use workshops

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.

Small tickets/PRs

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.

Sub-tasking

Before you start a ticket, sub-task and get it approved by another engineer. The advantages of this are:

1. You get a sense check on your approach ahead of time, avoiding blind alleys
2. You can define a clear approach before you start working
3. You get a little backup in a PR
4. 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.

KISS

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.

TODO comments

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.

Conclusion

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.