Code Review: good practices

What makes Code Review process good? What should you do, and what you should not? Let me share with you my experience on this 🙂

---

ℹ️ This article is highly based on internal content created by me for GetResponse during migration from Crucible to Gitlab, which was one of my initiatives during my first year at the company. I use “merge request” (MR) convention because I mostly work with Gitlab, but everything applies to pull requests too and code review in general, even if it’s done with Git patches sent via email 😉

The Purpose of Code Review

Code review, regardless to where it’s processed, has very important goal: deliver efficient, well written (readable, optimal, company-compliant), working code that satisfies business needs. From client’s or user’s point of view it’s not important what is the naming convention, coding standards and so on - it just has to work as expected.

It does not mean that Code Review does not have other goals, others are just less important 😉. Code Review allows us to:

• share the knowledge within team or cross-team projects
• learn developers with less experience with the language and/or the application
• find scenarios that code’s author did not think of before
• ensure that the general idea behind the changes is correct

General rules

Between author of the Merge Request and Reviewers/Approvers exists contract, which can be different within teams and projects, but some of general rules can be always applied. I am not talking about technical rules, but things that can make the process more efficient and friendly. Let’s look at it!

Be proactive

Keep track of the merge requests you’re participating. No matter if you’re author or reviewer, you really should keep an eye on opened MRs you’re participating in. Feel free (or even obligated) to ping author/reviewers for reaction if merge request is stalled, because the sooner it’s finished, the better. For example, Gitlab’s top-right menu has helpful items to simply go to the list of MRs and “todos” - you should observe these, and react if you have non-zero counters on them. In Github, there’s notifications view that contains everything what’s going on in the issues and merge requests in which you participate - keep it as empty as possible.

Be responsive

It’s really important to respond to merge request’s activity - do requested code review, answer comments, resolve discussions, push fixes etc. as soon as possible. Don’t ignore email notifications, if you feel that there’s too much of them consider changing notifications settings or create filtering rules. If you work in a distributed team, don’t mute communicator if there is no real need for it, so your team members can reach you if they need.

Be clear in communication

It’s really important to communicate well in order to not waste others’ time.

As an author of merge request, describe what your code changes do, how it’s working and how it should be verified. Write good commits (that could work as description), put comments in the code if something is complex or not intuitive.

As a reviewer write good comments that would clearly inform author about what you want. If there’s critical bug/mistake, just emphasise it. If there are some optional improvements or possible refactoring - write it too. But each of your comment should point what’s wrong and what is “definition of done” from your perspective. Avoid short and not descriptive comments that may trigger unnecessary discussion.

As any MR participant, if you feel that written discussion does not work, just arrange meeting to resolve it. Sometimes 5 minutes of talk can be much better than several comments that take more time to write/read. There’s also difference in perception, written words can be misunderstood and lead to unwanted clashes between you and others.

Be kind

Remember - you don’t need to agree with others, but respect them and their work. You’re entitled to point anything you want if it’s related to the code, project’s conventions, business requirements etc. Code review is not a place for personal animosities. Avoid passive-offensive wording, like “It’s bad”, these can do harm and make your arguments less convincing.

Be distanced

It applies mostly to merge requests’ authors, but reviewers also should remember that all the comments on code review are not there for attacking them, not for proving they are wrong or lame. Merge request discussions are the tool for improving delivered code. Don’t take the comments personally - you’re entitled to have your own opinion and conventions, so it’s natural that sometimes you will do something differently than others would expect. If somebody puts comment that it should be done otherwise, just discuss it without emotions (at least try to 😉).

Author’s role

As an author you should take care of the Merge Request from the time it’s created until it’s merged/closed, so you should assign yourself and keep yourself as an assignee as long as it’s you, who work on it. If anyone is taking the task from you and continue your work, MR should be assigned to that person (assignee always should reflect person responsible for delivering final product).

If you don’t have merging rights in the project you should be assigned to merge request until it’s ready to be merged. You are responsible for delivering approved code changes, merging is the last technical step, and you can assign different user to MR when it can be merged (but it can be merged without changing assignee too, you can just mention users with merging rights or add label - it should be part of your development process).

Draft / WIP

On most cases you should open Merge Request when code is ready to be reviewed, but you can create merge request and mark it as Draft even if it’s not ready. When MR is in Draft mode reviewers should NOT actively do code review, but only respond to author’s doubts or questions. Of course, they can do code review, but they should ask author if it makes sense - maybe author wants to change implementation completely and reviewing current code is a waste of time. When author thinks that code is valid and ready, Draft should be taken off and this is the time when proper Code Review starts.

Do NOT treat Draft status as a way to block merging - there is much better way of doing it by requiring approvals. Keep your branch up to date (by rebasing or merging with main branch) and use Draft flag only for initial development, not for whole Code Review process.

Reviewer’s role

⚠️ Most important: you’re responsible for merge request’s code quality and app security as much as author of the changes!

As a reviewer, your responsibility is verifying if delivered code changes are acceptable - in terms of overall good practices in programming, team’s and company’s standards and conventions, security and performance. You do it by checking MR’s diff and providing feedback, where’s required. You should use diff discussions or suggestions to help merge request’s author in providing best code possible. If you start discussion, you should keep an eye on it and mark it as resolved as soon as it’s updated by MR’s author and provided changes are satisfying (ℹ️ discussions can be automatically resolved if configured that way - this should be discussed within team/company). Remember, your comments will have higher value if you include:

• official documentation references
• links to specialised articles written by experts
• benchmarks showing advantage of proposed changes
• suggestion of changes (if your code review platform allows it)
• references to previous merge requests or comments related to currently reviewed changes

In terms of suggestions, use them in your comments to visualise what change you’re requesting, it will help the author understand you and even make it easier to apply. If you’re not familiar with the concept of suggestions, read Gitlab’s or Github’s documentation on this topic.

Code changes’ author(s) and reviewers are like small team, they should work together. Do not treat code review as your standalone work - keep an eye on other reviewers’ comments, provide feedback if you agree or disagree (maybe even more with the latter, because it can prevent incorrect changes). Reading other reviewers’ comments will also help minimise redundancy - without reading others’ comments you could add the same feedback and that would introduce unnecessary noise.

When code changes are acceptable for you, you should approve MR, that would explicitly tell other people that you agree to merge it.

ℹ️ Code review is not a place for pointing coding standards. Do not comment about indentations, braces’ placement and other things that should be automated using for example Easy Coding Standard. Just focus on actual code 🙂

Technical details

It depends on the platform where code review is done, but some technical rules apply nonetheless:

• Title should reflect provided changes. Ensure title is clean and descriptive. If there’s naming convention (e.g. with JIRA ticket ID), use it. Use Draft to indicate that code is not finished yet (if applicable).
• Use description for providing additional information for reviewers. You can describe background of the change, explain why it’s implemented this way, how to verify it manually and so on. Put there everything that could be helpful for better and faster code review.
• Assign yourself to Merge Request, that will make easier to track its progress
• Assign reviewers for code review (if you don’t have this automated). If project allows it, you can fine-tailor approval rules - it’s recommended that 2 approvals are required to allow merging, but in more complex MRs you can demand more approvals, also from specific people.
• If your merge request is related to other MR(s) you can define merge request dependencies which will prevent merging your changes before merging other MRs.

Summary of recommended Code Review flow

• Configure your projects’ settings in order to optimise merge request flow (approvals, code owners, requirements that must match before merging).
• Automate everything what’s possible from QA perspective (coding standards, static analysis, tests, linters etc.) so you can focus on actual business logic and/or technical change in Merge Request.
• Open MR as soon as you need feedback (from CI or from reviewers), but mark it as Draft if it’s not ready for final review. Remove Draft when you think it’s ready (it indicates that you finished your work and want feedback).
• Assign reviewers when merge request is in state that requires feedback (Draft with things to discuss or when it’s ready for review).
• Keep MR open as short as possible. Opened merge requests in most cases should have higher priority than other work in order to finalise it and deliver product.
• Keep in touch with other participants - author and reviewers should work as team responsible for finalising task. Track merge requests where you’re participating, make it your daily routine.
• Merge MR when it meets all requirements (green pipeline, discussions resolved, required approvals gathered). Sometimes it’s a good idea to rebase your branch before merging, but it depends on project’s flow.

ℹ️ Code Review topic is really wide, I focused here on soft side of it. More technical follow-up posts will be published in the future, so stay tuned and come back here regularly! 😎

Happy reviewing!