This is the first post in a two-post series in which I will discuss the code review best practices we use in our team (AI Ops) at Unbabel. Currently, most code at Unbabel is reviewed and approved by at least one other person. There are a handful of exceptions to this for demo projects and limited changes where automatic checks are sufficient. We use Jira for issue tracking and GitLab for our repositories so you will see references to those in the discussion below.
So why go through the code review process? It adds to the total time to merge a change in the trunk. Plus it consumes non-coding time from both the MR creator and the reviewer. A well-designed Code review process provides the following benefits:
- Checks code for correctness, consistency [1], and quality
- Ensures the code change is comprehensible to other engineers
- Promotes team ownership and knowledge sharing
- Provides a historical record of the code review for the future
I would say that in most cases, the benefits of code review are worth the resources it requires. But the keyword here is “well-designed”. If you don’t have a code review best practices defined in your organization and a culture to follow them, this process can be more pain than pleasure. If you find your engineers asking, “Why do we even bother with this process?” then something needs to be fixed.
Our best practices aim following goals for the code review process:
- Improve the speed of the code review process
- Improve code review quality
- Minimize the cognitive workload for both creators and reviewers
- Leaving a better history
- Increase Developer Happiness
In this Part 1 I will discuss the best practices we recommend for MR creators. In Part 2, I discuss the best practices for MR reviewers.
Best Practices for MR Creators
Keep the MR short
Ideally, the MRs are around 200 lines or less. This is not a hard rule since you can’t always avoid longer MRs but do everything you can (within reason) to keep the MRs short.
Shorter MRs mean faster reviews and less cognitive load for the reviewers.
Plus short MRs are much easier to test.
Tests included
Goes without saying but when adding features or changing them, your MRs should include tests
We won’t go into details of what good tests should have (perhaps in another post). But one key characteristic to highlight is that your tests should make it obvious what behavior you are trying to change and the new expected behavior.
MR title and description
Your MR title should be clear and give a very brief summary of what your MR is doing.
Additionally, your MR should have a detailed description of:
- What is your MR doing? Is this a bug fix, a feature, a code-quality improvement, or a refactor?
- Why is it important?
- What were the design choices and the reasoning behind your final implementation choice?
- What are the main changes to the code?
- Are there any specific changes the reviewers should look at?
- Do you have specific questions for the reviewers or concerns with the current implementation?
Since we use Jira integration, we also add the Jira issue name to the MR title so that we can automatically link GitLab MRs with their corresponding Jira issues.
Link additional (Jira) issues
If your current MR is part of a series of smaller (safe-to-merge) MRs solving a problem or adding a feature then you should add that information in the MR description. Doing so will help you avoid unnecessary back and forth with your reviewer about missing steps and how this particular MR fits in the large scheme of things.
Additionally, if you find logically separate work during the MR review that needs to be done then create a (Jira) issue for that work immediately.
Adding reviewers
At Unbabel, some teams manually assign reviewers while others use an automatic bot to randomly pick reviewer(s) from a select group. Regardless of the mechanism you use, I suggest only assigning one or at most two reviewers for your MR.
Assigning more than one reviewer can create a sort of “bystander effect”, where each reviewer might think that other reviewers will review your MR.
There might be a case where you need to add more reviewers to review specific parts of the code change. In this case, you should be explicit about the part you want to review.
Share your MR
We often share the MRs in a public channel (of the team or the repo-specific channel in Slack). Even if you have manually or automatically assigned reviewers, you want to advertise the change you are making.
The purpose of sharing your MR with other developers is to highlight your work and also inform people of the change for knowledge sharing.
Don’t just paste the link to the MR. You want your message to summarize the change and why it’s important, in a sentence or two.
Use synchronous communication (but only if needed)
First of all, your code should be easy to comprehend so that your reviewers would not have a hard time understanding the change and the reason for the change.
Optimizing your code for comprehension rather than for brevity at the expense of readability.
But some changes are just complex. If there is still some confusion and/or significant questions, then it’s best to discuss and clarify those questions using a synchronous communication medium (video call or an in-person meeting). This will save you a significant amount of time and frustration.
After the synchronous communication session, summarize the discussion and add this to your MR overview.
If you work in a globally distributed team, the necessity for your MR to be self-sufficient in explaining the change and answering any major questions that your reviewer might have becomes even more important. Synchronous communication is simply more expensive in terms of time and effort. So if you can avoid it, it’s better.
Create a history
Keep the discussion about the MR on GitLab/GitHub (or whichever platform you use).
We avoid discussing the MR in Slack/IM/emails, as much as possible. The reasoning is that all the discussion about your MR is likely something that someone (or even you) in the future would need to refer back to. But if you do, move a summary to the MR overview/comments (just like you should do for the discussion from any synchronous communication). I am sure you have into code that had no MR overview or comments in the MR so it was hard to understand the decisions made or what the code was doing.
Your organization might rely more heavily on emails. Where you leave the history is not as important but just make sure that all the discussion about the MR is easily accessible for someone else, potentially even years later.
Leave the codebase better
Lastly,
Always leave the codebase better than you found it.
This could mean a small fix to the documentation, fixing a typo, or renaming a function or variable. Don’t hesitate to do those things in your current MR. Otherwise, these small changes are often postponed in practice.
Just remember to highlight the reason for this change in your MR overview.
[1] Invest in setting up (automated) consistent formatting and styling tools (linters and formatters) for your company so that all codebases in your company are consistent and no one needs to do this manually. Automate the boring tasks and ask your engineers to do the more engaging ones.
What are some of the code review best practices for the MR creators that you use at your organization? Do you have a code review for every code change or are you using ship/show/ask or a variant of this method?
