Welcome, if you're here reading this post it mean you already contributed to some projects where you were not alone or you plan to. If you want to know more about what we call the code review AKA code contribution reviewed by peers before merging it to a common trunk.

Mindset

In this process the main purpose is to challenge a technical response to some problematic in the project context.

If a reviewer suggest another approach, you must ask yourself, is it a better approach than yours? If the project can benefit from the suggested change in any domain, user experience, developer experience, security, performances or code clarity you must consider it seriously and push the asked changes, otherwise at least ask more information then also engage a direct chat if you need.

The whole process should be taken like strictly nothing but a win win, the better you care of the review process the better the produced code quality will increase thus the overall project quality will benefit as well.

The code hygiene and stability is the main quest, you guaranty best practices, security, maintainability and performances. Sometimes on some projects this is not already the case and you have to maintain a legacy project anyway? Apply strictly the same process to all the new contributions then you can leverage usage of a technical roadmap to clean what's need to be cleaned.

Of course but important to mention again be respectful, kind and never get upset unless you're Linus Torvalds. Be open to suggested changes.

Interact with reviewed developer

Using most popular git web interface such like Github or Gitlab there's a lot of way to interact with the code author. Sometimes the change you request are not mandatory, or can be postpone on some technical roadmap, sometime the contribution will alter project's code quality, security or performances directly at merge so they must be addressed before.

When you open a thread, and you ask the author to implicitly push some changes. In that case, a suggestion is always more easy to understand rather than a long explanation without one.

it's your responsibility to follow up and resolve the thread as soon as possible to avoid blocking the next steps in the process. Using the most common web apps such like Github or Gitlab you have administration panel where all your waiting reviews can be found, be sure to use those pages on Gitlab it's https://gitlab.com/dashboard/todos and Github https://github.com/pulls/review-requested.

Sometimes the best way to understand each other is to simply start talking in person or onto a one to one online chat.

Project size considerations

The more contributors your project have, the more tight the review and merge process must be. Be careful to not be overkill but also not be to permissive.

At least 20% positive reviews from the whole contributors with some robust CI steps about code audit, lint and tests must be sufficient. You can leverage role notion of all git web app user system to ensure at least one core maintainers approval for projects with many contributors.

Again your process your rules, at least they are documented somewhere. So pull/merge request title format, description related meta information along with any other rules are the plinth of this process.

The process

First of all the smaller the contribution are, the better this process is. Avoid TLTR reviews (Too Long To Read) by splitting your task scopes onto little chunks.

Checklist

• First of all, you take time to review a merge request, the developer who assign you must deliver a stable branch and finished contribution, all CI code quality related steps must turn green.

• An other important point is to check that the reviewed code was correctly rebased on top the target branch HEAD and thus contain no conflict at all. Otherwise ask the contributor to rebase his branch before reviewing the code.

• The code lint must be valid, all kind of test suites must pass and all code quality analysis tools. Otherwise ask developer for related changes before reviewing the code.

• Now it's time to analyze the changes AKA which files were edited or added. You need to have a global point of view first. The more the developer add context, like explanation or link to ticket, even medias like video or screen capture, the better reviewer will understand the purpose of his code, so here again document or override git commit template to setup the standard you need with your team.

• The code add a new feature, or modify an already existent one?
  It's a fix? Anyway this code should be backed by any kind of test to ensure it actually work and fail as expected and will not regress later.

• Did this contribution enough documented? Is the spec properly updated?

• Did the contribution properly handle errors or anything that could break the normal code behavior. Is it possible to understand through log the application behavior.

• Can this contribution be simplified in any way?

• Is this contribution at the right abstraction level?

• Does this project has some serious code analysis steps, is the test coverage good at least 80% code tested and critic user stories scenarios tested end to end? If not, you must checkout the branch locally on your current development environment, then rebuild it from scratch. At this level ensure all the project code analysis tools available for code testing, quality and lint. By the way a clean CI/CD is the core of a clean project.

• Do this code alter the database and all persistance layer, is the migration process okay when the CD process will apply it to production staging with already tons of data?

• Is this contribution can expose unwanted sensitive data?

• Is this code altering in some way the global project performances, there is a risk with a high volume of data?
  Does new column indexes are needed? Will it scale?

• Is the contribution safe or this contribution will introduce a new security flaw? Bad practice, potential XSS vulnerability, unsafe value versioned on git, backdoor exploit added by malicious contributor. Again some tools are available to check security flaw in your CI pipelines.

• Then any project has some contribution related rules, sometimes in a markdown file called contribute.md at project root. Does this contribution respect it?

Do and don't

Do

• Respect the previous checklist in the same order, the code review must occur when a developer assume his contribution ready.

• Take the time to review yourself first.

• Ask a the reviewed developer to flag his contribution as WIP or draft or anything that can tell other reviewers that the code is not ready to review if it's the case.

• Add some context on your thread, sometimes you're request is not that obvious for a developer. Saying that this way to do it, is more clean or a better to do it without giving an explanation. Example: Can you split this service onto two another, one in domain X and the other in domain Y, to avoid mixing those two responsibilities.

• Ask for small contributions, better split your contribution and the task scope on little chunk, to avoid TLTR reviews.

• Always assign the best reviewers possible for your contributions, find the expert of each part of the stack you touch.

Don't

• Assume that a developer is too good and so senior on project, that you can approve eyes closed his contribution. No one is perfect, even AI make some stupid mistakes, nonsense and errors. If you do so you're against the main purpose of this process.

• Review a WIP code, such contribution should be flagged draft, without review requested.

• Approve not tested contribution

• Approve commented code

• Approve not used code

• Approve something you don't understand

• Approve some kind of hack or workaround to fix a bug

• Do not repeat yourself for a typo, the developer made a typo in a wording and this same typo is everywhere is his code. You don't need to open a thread by typo and 50 threads related to this single typo. You will loose your time and this will not help at all the reviewed developer also if there's also other threads.

Gate keeping

One phenomenal that you will encounter is known as gate keeping, in other terms someone very strict about your contributions, or yourself wanna every contributor to code strictly like you want to.

The best way to deal with gate keeping is to use strict rules and provide documentation about it, especially about contributions.
Otherwise enhance the developer experience via code quality analysis rule sets, templates, lint and all kind of quality control steps directly on your CI process.

Most of the time gate keepers are more like psycho rigidity concerning the project rule, again if it concern the project rules and not his own gate keeping rules thank that reviewer for doing the core purpose of this process and do it well.

Word of end

Keep in mind that code reviews are actually the best and quicker way to progress as a developer. Some simple comment added by generous reviewers can help you to improve a lot your skills sometimes.

Thank you for reading, feel free to comment as well your best practices for reviewing code in the comment section below.