Code review: Best practices and techniques

Basically, someone else will assess your work looking at any issues that you might have missed (and providing a second opinion on your solution). The problems that the reviewers are looking for during code review are usually bugs, faulty logic, security or performance issues, etc.

The code review happens before the proposed changes are integrated with the rest of the codebase. Usually, this means that the developer wants to merge his feature branch with the main branch. Code review is a “blocking” step during the development process, because the work will stay in this phase until all of the reviewer’s remarks have been addressed.

There are several main reasons why code reviews are an important part of the software development process:

• Ensuring that code quality remains on the same level over time. This includes many things: respecting coding standards, using best practices, taking performance and security into consideration when designing a solution, etc.
• Discovering (and fixing!) issues early
• Sharing knowledge between the team members and increasing collaboration (because people are working together to improve the proposed solution)

Possible obstacles

However, there are also some drawbacks when it comes to code review:

• It takes time and effort to do a code review. This will take focus away from whatever else the reviewer was doing. This can cause a reviewer to do a poor code review, just to “get it out of the way”.
• As already mentioned, code reviews are blocking the software development process. This means that it takes longer to deploy your changes to production. Sometimes, a review could be complex, or a reviewer could become a bottleneck (due to various factors) which will result in review taking more time. Because of this, sometimes the team could start thinking about “skipping the code review this time”, so they can deliver things faster.

I think that these drawbacks are especially important for the discussion, because they clearly illustrate the biggest issue of using code review as part of your software development process – being inconsistent. Code reviews will provide the most benefits if you do them on a regular basis and in the same way. 

Doing code reviews the right way takes discipline. If you are doing them occasionally (for whatever reason) or if you are changing your approach every time – you are simply doing them wrong. You either do them consistently or not at all. Yes, there are some situations where a code review might not be necessary, but those are exceptions.

Code review guidelines

And this brings us to our next topic – defining code review guidelines. How do we know what to look for during the code review? This is especially important in teams with mixed seniority levels. 

Even in a team of experienced engineers, different people can have very different opinions on what is good code. Because of this, your team could define a set of guidelines for doing code reviews, so everyone can assess the changes in a similar way. This will ensure that there are no large differences in review quality between the team members and establish a minimum set of requirements that must be met. You can even define separate guidelines for authors and reviewers. This will help every team member to submit and review changes in a consistent way.

Below you can find some examples:

The solution is in line with your coding style/standards

If your team has a policy that defines things like class and variable names, method signatures, application structure, logging practices, etc. – make sure that the author is following the rules imposed by this policy.

Changes are logically organised

When submitting your changes, make sure that you group together related changes and write your commit messages accordingly. Lumping 35 changes over 10 different classes together and labelling them as “small change” is sloppy. This will only make your reviewer’s eyes bleed (and your Git history will look bad).

Build/pipeline must not fail

Never submit code that doesn’t work. If there are build errors, or your CI/CD pipelines fail for any reason, resolve these issues before submitting a PR. On the other hand, if the reviewer receives such PR, they should immediately return it to the author until he fixes all the issues.

A brief description of the change must be provided

If you open a PR, make sure that the reviewer has some way to figure out what problem you were trying to solve. This can be done either by providing a description of the change or by linking the corresponding ticket (which should contain sufficient level of details). Without this, the reviewer’s job becomes much harder, because he will spend much more time trying to figure out what the code he is reviewing needs to do.

Code analysis tools report no errors

Code analysis tools like SonarQube can be helpful, because they can detect some overlooked issues and suggest different ways to improve your code. If you are using such tools, you could require every feature branch to be analysed before the PR is opened. Any issues (if detected) must be fixed in order to finish the review.

Code is covered with tests

If you are writing tests on a regular basis (or cover most of your code with tests), then no PR should be submitted without tests. If something like that happens, there should be a good reason for that. So make sure that you let the reviewer know why you are not submitting any tests.

PR does not contain unrelated changes

The PR should focus only on the problem that you are trying to solve. Including any unrelated changes in your PR will not only confuse the reviewer – it can also make the QA process harder because these undisclosed changes could introduce some new behaviour. Avoid doing this only because you think it’s convenient and you want to “kill 2 birds with one stone”. If you believe that you should include an unrelated change together with your PR, discuss that with your team.

However, there are a few things that you should try to avoid when conducting code reviews. Let’s look at some of them:

Doing review without having the proper context 

If you do not fully understand what you need to review, then you won’t be able to decide whether the submitted solution is good or not. If you are not familiar with the task that you should review, then you must do some research and reach out to the author to understand the solution and motivation behind it a bit better. Otherwise, you might end up focusing on the wrong aspects of the problem or missing some things that, while they seem fine to you, do not work within the related context.

One-man show

Try to avoid situations where a single person is conducting all code reviews. Sometimes, this can be the case with very small teams, or teams where the seniority level is low – but even then, it should not stay like that for long. Code reviews should be an activity in which the whole team participates, as mentioned earlier. Having only one person acting as a reviewer is not a good thing for several reasons. The first one is that the reviewer could impose his own personal style as the “one and only”. The second one is that this person could end up stuck in doing too many code reviews and become the bottleneck for the team. This can happen because everyone is waiting for a single reviewer to finish the review.

Proposing large or unrelated changes during the code review process

If something like this happens, you need to have a good reason for doing it. In general, doing a large redesign in the middle of the code review could mean that the author did not understand the context properly, so some aspects of the problem were not covered at all, or that during the review it was discovered that there is a much better way to solve the problem.

These things can happen – but rarely. If this is a common thing in your code reviews, then you need to investigate it. It could mean that the development team does not really understand the problems that they are solving. However, you should avoid adding improvements to the code that is being reviewed “just because” or because it is a good opportunity to tackle some other issue as well. At that point, you are introducing changes that are either not related to the task at all or bring very little value to it. This will make your history of changes hard to follow. These changes deserve a separate task and their own code review, so it is perfectly clear why and when you did something.

Weaponised code review

This is a situation where the reviewer intentionally looks for issues and criticises the work of the author, because they have personal issues with the author. It is an extremely toxic behaviour, and it can severely affect not only the author, but the whole team. Issues with other team members should be resolved elsewhere – the focus of the code review should be only the code, not the person who submitted it.

Conclusion

As you can see, conducting a code review properly is not easy. It is a complex activity that takes time and must be done consistently (you can even incorporate it in your product development life cycle). However, it can have a huge impact on the overall quality of the product, team collaboration and knowledge sharing. Because of that, it is worth investing some time and effort in setting up a good code review process. The benefits you can reap from it are much larger than potential drawbacks.

---

Author

Miroslav Lazovic is an experienced software engineer from Belgrade. Over the past 15 years, he worked on many different projects of every size – from small applications to mission-critical services that are used by hundreds of thousands of users on a daily basis, both as a consultant or part of the development team.

Since 2016, he is focused on building and managing high-performing teams and helping people do their best work. He likes discussing various important engineering and management topics – and that’s the main reason for this blog. Besides that, Miroslav has quite a few war-stories to tell and probably far too many books and hobbies. There are also rumors of him being a proficient illustrator, but the origin of these rumours remains a mystery.

– Backend TL@Neon and Dev. Advocate @Holycode