A Step-by-Step Guide to Effective Code Reviews

Code reviews are a practice that has become widespread in recent years, where one or more developers review the new code implemented by another colleague, with the aim of detecting code quality issues, bugs, vulnerabilities, bad practices, etc… . This allows feedback loops to be shortened, which we know is very beneficial because the later a problem is discovered, the higher the cost of fixing it and the greater the potential business impact.

As code is the essential material from which software is built, and is created and maintained on a daily basis by different developers and teams, it is vital to have mechanisms in place to ensure its quality. Conducting good code reviews, consciously and according to best practices, can make the difference between the success or failure of an application and, ultimately, a business. Therefore, one of the first recommended steps in adopting code reviews is to have a how-to guide to provide guidance and direction to the whole team. This article is intended to serve as an initial guide that teams can use and adapt to their context.

Fundamentals of good code

Before going into details, I find it interesting to share this document, that although it is called Zen of Python, it is a good guide for any language.

The beautiful is better than the ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability is important.
Special cases are not so special as to break the rules.
Although practicality beats purity.
Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, reject the temptation to guess.
There should be one– and preferably only one –obvious way to do it.
Although that way may not be obvious at first unless you’re Dutch.
Now is better than ever.
Although never is often better than right now.
If the implementation is hard to explain, it’s a bad idea.
If the implementation is easy to explain, it may be a good idea.
Namespaces are one honking great idea — let’s do more of those!

Source

Objectives of Code Reviews

• (Style) Detect divergences from the style defined in the style guides (it is advisable to define style guides for the code of our company) and in the code of the project.
• (Architecture) Check if the implementation follows the architecture of the code.
• (Modularity) Check if the new code is too tightly coupled. Check that the components have cohesion, have a single purpose.
• (Composition and inheritance) Review whether inheritance and composition are used correctly. And priority is given to composition over inheritance.
• (APIs) If APIs are created or extended, check that they are consistent and clean.
• (Readability) Check that the code is easy to understand, follow and maintain.
• (Testability) Check that the code has unit tests and automatic tests that test the new functionality.
• (Preventing bugs) Detect possible bugs and/or cases not covered by the code.
• (Knowledge sharing) Familiarize the rest of the developers about the changes that are being made in the code. It allows the knowledge to be more distributed in the team.
• (Training) Transfer knowledge from senior programmers to newcomers and juniors.
• (Security) Check that the code does not expose credentials of any kind, and has no obvious security flaws.

Setting the ground

The objective of the review is not to criticize the work of colleagues, but to review it in order to improve it together, to detect possible lapses or bugs, and to learn through feedback. Comments should always be respectful and should be received in an open manner, thanking our colleague for the time he/she has spent reviewing our code. The review should never be approached as a competition between adversaries, but as a team work.

No matter what seniority we have as developers, we are all human and we all make mistakes. A good developer, no matter how senior, is always open to feedback even if it comes from a more junior developer.

Steps to follow

Pre-review

1. Open the Pull Request/Merge Request (or PR / MR) and read the description. If you still lack context for the change, open the ticket in your project management software and read it. It is important to read the acceptance criteria, to try to detect if the code can meet all of them or not (of course, the purpose of code review is not to test it, but in many occasions you can detect uncovered acceptance criteria simply by looking at the code).
2. Verify that the base branch is the correct one. Sometimes by mistake, you can create a PR against main, when you wanted to integrate it in another branch. In case it is not, notify it and stop the revision. Because if the base branch is not the correct one, we may be revising changes already revised.

Review

1. Start by reviewing the list of changed/created/deleted files. Check that the file and module architecture is consistent with existing and best practice guidelines (if any). Check that the names follow the naming convention.
2. If the code has unit tests (recommended), starting with the unit tests can be a good way to understand if all the acceptance criteria are covered or not. It is also a good way to understand the code from the outside. Unit tests should cover not only the happy path, but also corner cases and error cases.
3. Check that the style of the code follows the style guide (if any), if not, check that it is uniform with the existing code. Check the names of variables, classes, methods, etc…
4. Check that the architecture of the new code respects the existing architecture. For example: new classes are consistent with existing classes. The way the code is organized in files follows the existing patterns.
5. Check that the code is loosely coupled and has cohesion. Classes and methods should have a single purpose. A common example of high coupling is where access to the database engine for queries is spread throughout the code. This leads to high coupling to the specific database engine, making it difficult to replace in the future, for example.
6. Check that no code has been duplicated. DRY (Don’t Repeat Yourself)
7. Revisar que el código es sencillo de entender. Si vemos código complicado, deberíamos añadir un comentario a la PR, para que el desarrollador intenté hacerlo más sencillo y/o añadir comentarios para que se pueda entender.
8. Check that magic numbers or literals are not used, which makes the code less understandable and less maintainable. Suggest the use of constants with a self-explanatory name.
9. If an API has been modified, check that the changes are consistent with the rest of the API, and check that the documentation has been updated.
10. The idea is to review the new code, but sometimes to understand it well we will need to read a little bit the context, the already existing code. Sometimes we can see that the change is incomplete or not correct by looking at the context.
11. Check that the code does not include passwords, credentials or sensitive information of any kind. If so, this information is already in the repository and could be compromised, consider changing the credentials.
12. Be on the lookout for any security issues that are obvious to the naked eye. For example, sql injections (using a user input directly in a sql query).
13. Review whether the changes should reflect changes in the repository documentation (e.g., in the README), and see if it has been updated and is correctly understood. For example, if the way to launch the project locally has been changed, it should be clearly updated in the README.

Post review

1. In the event that we have made comments, be attentive to the possible response and changes in the PR, to review again.
2. If we have not made comments and we see it well. Indicate it in the PR and accept it. Here, depending on the rules that we have defined in our team, it may be necessary that other people review it and follow a specific process to merge the PR.

Final notes

This guide serves as a generic reference for making code reviews in a uniform way across a team/organization. The idea is that each team/organization can customize and extend it to their particular case, and work continuously to improve and adapt it to their circumstances.

In addition to all these manual checks, it is highly recommended to incorporate static code analysis tools, linters, and formatting rules in the IDE, among others, into the process. These will allow us to detect problems, quickly and cheaply, even before creating the PR to request the code review.

And of course, your comments are always welcome.