This blog post was originally published on the GitLab Unfiltered blog. It was reviewed and republished on 2020-09-15.
Like most companies, code review at GitLab is a major part of our workflow. But it's clear from the results of our 2020 Global DevSecOps Survey that code review can be a major reason for delayed releases and overall frustration. A vast majority of companies conduct code reviews (some even on a daily basis) but that doesn't mean it isn't a potential time sink.
How to perform a code review?
But code reviews can be done efficiently, and I know this since I've been a maintainer for 3 years. Here's a look at my top four tips for code review based on a tried and true routine that allows me to do effective code reviews and merge code quickly and efficiently to aid in others not being blocked by me. Of course, this is what works for me – your mileage may vary. Here's how I do it:
Tips for code review no.1 - Time management
An early start to my day makes it easy to start reviewing merge requests first thing. I set myself a time to start reviewing and I will keep at it until my GitLab "to do" list no longer has any merge requests that need reviewing. Mornings work for me; it's the time of the day when I can focus the most and get the reviews done with minimal distractions.
Getting to reviews after this time is hard. I have other work that needs doing as well so once I've reviewed all merge requests on my list I leave anything new until the next day. Of course, as with all rules, it ends up getting broken. Depending on the size of merge requests, I may make sure I review them before my day ends to make sure anyone in other timezones aren't blocked by me.
Tips for code review no.2 - Unblock others first
It's not great for the author of a merge request to have to wait X hours/days before they get feedback. The sooner they get feedback, the sooner the merge request can be merged and shipped. Making authors wait just creates uncertainty and may mean that other work gets held up.
This is why I find it important for me to review a merge request as quickly as possible. At GitLab we have a 2 day Service Level Objective (SLO) for feedback from reviewers. For myself, I always try to do better than that and respond within a day.
3. Tips for code review no.3 - Focus on the code, not the feature
This is going to be a point that could create a lot of discussion: Instead of focusing on the feature, focus on the code.
A lot of the merge requests I review are across different groups, with features that I don't fully understand or with features I have no way to test. I could spend a lot of my time reading into the feature and the issue to understand what it is, but that means spending more time not reviewing everyone else's code. Also, if I did this with every merge request, it would be hard for me to keep to my time limit.
Who is better to review the feature itself then? The product designer (UX) or the product manager both understand the feature being worked on and are better suited to help find bugs or guide the feature in the correct way. It is important that someone in the UX team review the feature to make sure it matches the designs and vision they had created for the feature. If a merge request has no UX review by the time I get to reviewing it, I will normally ask the author (or ask a product designer myself) to have the UX reviewed before I merge the merge request.
However, this point is also something I don't always stick to. If a merge request is touching an area that I am familiar with and I can tell from the code that a bug exists, I will test it locally and provide as much feedback as I can to help the author understand the bug. The more you – as a reviewer – work with the code, the easier finding bugs through the code becomes. I have been working on the GitLab codebase for over 4 years, so seeing where bugs could arise through looking at the code has become natural to me.
Tips for code review no.4 - Seek to understand: Ask questions
It is easy to suggest changes to the code that I am reviewing, however sometimes what I suggest may not be right. It is important that instead of just suggesting a change, you always try to ask if the author thinks it is the right change. Having a conversation around a change helps both the reviewer and the author understand the existing code as well as the code being suggested. Maybe the suggestion had already been tried by the author. Being open to talk about it helps get to the final solution.
Sometimes however suggestions for changes happen around legacy code, i.e., code that has existed for a long time without being updated to match our documentation. In these cases, the conclusion may end up being that a technical debt issue be created. This is ok. We should strive for boring solutions first but also understand that a more optimal solution may be required in the future.
To sum it up
Reviewing code efficiently is a skill that gets learnt the more you do it. Spending time coming up with a workflow that works for you is just as important. Over the years I have been reviewing code, I have stuck to these tips as closely as possible. Yet, I am far from perfect; I am constantly learning about new and different ways to optimise my workflow for code review. I would love to hear other tips and workflows. It is through discussions that we can improve and push ourselves to be the best that we can be.
