How to be good at Code Reviews

In my time as a developer, I have given hundreds of code reviews. It's something I learned to enjoy very much as it gives me others' perspectives on our codebase. Most days I learn something new from it. In this article, I want to go over few tips on how to be good at them and especially how to enjoy them.

Good pipeline

Most teams have some kind of code-style rules and recommendations on how to uniform their codebase. Whatever you do, do not force code reviewers to check whenever the pull request complies with those rules. There are tools to do that look at eslint, SonarQube. These tools should be run as part of your PR pipeline.

SonarQube is one of those tools that will improve code reviews for everyone in your team. It can find style issues but can do so much more. With their code analysis, they can find the most common mistakes you can do in your code. You never closed your resource? Sonar will let you know. Personally, I never do code review before Sonar finished its job.

You need to have a concrete process that will run every time somebody creates a PR or pushes new changes in there. Steps can look something like this.

• Compile the code
• Run the tests
• Run linting
• Run SonarQube
• Do the manual review process
• Merge to the main codebase

Flyover

Like first thing, in my review, I will do a quick overview of all the changed files. This I usually do inside the pull request UI. I am focusing on few things during this stage.

First is the code readability. How is the reading experience? Is it apparent what is being done? Good code should be able to convey its purpose at first glance. Then I am trying to figure out if this change is what is required by the corresponding change request. I will read through the ticket and try to match all requirements there in the pull request.

During this stage, I do not dive deep into the implementation details. I identify those for later stages.

Possible points conveyed at this stage:

• change is not very readable
• change does not cover the requirements, or change request ticket was not changed as requirements changed
• missing tests

Go deep

After the brief code flyover, I have a list of changed files I need to go deep into. I usually do this in IDE so that I can see connections between files better, but for most changes, it's ok to do it in the web UI of the pull request.

I start by asking the question of how would I solve the problem and go from there. Is there a piece of code in our codebase that does something similar (or the same)? Is there a library (that we currently use) that can be used for this? If there is a library that we do not use currently is it worth adding to solve this issue?

Test review

Do not forget to review the tests. Are all cases valid? Does those tests have the possibility to find something? Are assertions readable? Are tests even present?

Do not be afraid to fight back to add more tests if you as a reviewer feel like there is a possibility to add them to improve the codebase.

Be nice and educate

Whatever you do, do not be arrogant, snarky, and sound like a know-it-all. If the proposed solution is valid, but you would do it differently you may say so but also approve the request. You may leave education comments, but too much can of those can be harmful. Consider what feels important. Do not argue about little things, people are different and have different opinions.

Approval

Be quick on approval, not perfect is ok.

This is the golden rule you should go by.

For more tips like this, you can follow me on Twitter.