Working in a team and doing code reviews are two of the most challenging parts of engineering jobs. Mostly that’s because teams are composed of actual people, and each of those people has a different past, a different background, a different culture, a different mindset, etc.

Some people care about their work and invest time in doing things as best as possible, while others don’t give a damn about what ends up in the codebase.

Each of us has developed his way of reviewing the code, and there is always something more to add to the local knowledge. For example, some do the code review in an IDE, while others do it on platforms like GitHub or BitBucket. Doing the code review in the IDE makes you quickly navigate the pull request files and enables you to quickly sketch up some ideas.

Are you in the right mindset?

Before starting to review people’s code, take a break or do something to relax. Take a coffee break, make random conversations, and have a few rounds of a 1-minute plank. Anything but engineering should work.

You should be prepared to face the “best”. And to make things easier and more productive for everyone, a great idea is to be detached, away from personal emotions and beliefs, ready to judge whatever will come from a 3rd person’s perspective.

The tricky part is the one with the 3rd person’s perspective, and probably you’re wondering why I’m mentioning such absurd “esoteric” things. Well, the code reviews are not just about the other’s work but also about you and your opinions, your beliefs, and your past experiences. Before suggesting something, judge your idea like another person would review that suggestion. Take the idea and evaluate it from many angles. At the end of the process, there are high chances that your proposal might not be meaningful enough, the benefits would be insignificant, it might create confusion, it might not adhere to standards, or it won’t resolve any problem. Or, you might end up with even more positive reasons to do it or even more solid arguments you never fancied about.

The PR should have a ticket ID

Usually, it is either in the PR’s title or description. If not, request adding it. The ticket ID links the pull request to related resources, such as functional and non-functional requirements and associated tickets. For example, it can come in handy if you need to find out why a specific decision was made.

Get a high-level understanding of the PR

Try to understand the pull request from the ticket title, description, and, if present, the attached links. At the end of this step, the reviewer should be able to clearly state the objective of the pull. If it doesn’t happen, then leave a suggestion to improve it.

Check commits descriptions

The description and the chronological nature of the commits complement the understanding steps and should describe a logical, rational, one-way path for achieving the goal of the pull request. After this step, the reviewer should be able to partially identify the components of the new code and the interactions between them, and it should be able to state the steps taken to achieve the goal.

The commits need to contain as much information as possible related to the changes they do. For ex., in case of defects/bugs, the commit description should have at least the following: the link to a ticket, the main change description, the actual behavior, the expected behavior, the analysis of the problem, the proposed solution, and links to other relevant resources.

Is the PR too big?

If, as a reviewer, the PR might have too much code, consider suggesting to the author to split the PR into multiple PRs or multiple commits. Small PRs are more likely to get approved because they are easier to review. This is very subjective, and it is not always possible.

Check if the source branch and target branch are correct

Sometimes people choose the wrong target branch, especially when their way of working involves feature branches. Nobody wants to see their code with database migrations go to the main branch when it should’ve been in a feature branch instead and deployed in a dedicated namespace.

Is the PR crossing domain boundaries?

By looking at the modified files and directory/package structure, the reviewer can quickly identify what areas of the application have changed.

The goal is to identify if the pull request touched code from other business domains.

Issues that can be spotted at this step:

• dependencies were established between business domains that should never be connected
• dependencies were established with a child domain – the rule is that only the child domain should access the parent domain

In practice, people import entire modules just because they need a single utility function available there in the module, thus creating solid dependencies between domains without even realizing it.

Analyze the tests

Looking at the tests before checking the implementation can provide plenty of insights about the new code.

The functional tests should cover all the acceptance criteria of the feature/bug and offer the reviewer a good understanding of how the new functionality integrates with the system.

The unit and integration tests should cover functional and non-functional specifications. They should offer insights on technical details such as how the components interact, their constraints (ex., wrong parameters), or potential errors (ex., exceptions).

Are functional requirements fulfilled?

The reviewer should confirm that the main code fulfills all functional requirements and that the code is in accordance with his understanding created based on the previous steps.

Are any non-functional requirements that the author should consider?

Think about performance issues, latencies, resilience, availability, and other potential technical issues that might result from merging the new code. One of the frequent cases related to relational database migrations is that people often forget to add indices to the columns they query. Depending on the data, such an omission can put down an entire service, even the whole platform.

Are there missing tests?

It frequently happens that engineers omit test cases, with or without intention. A high-quality code should cover every possible corner case. That’s not always possible, and, at some point, we have to make some trade-offs. If it’s not a horrendous effort, suggest adding the missing tests.

Do you see any corner cases?

Especially when TDD is not applied, engineers will likely miss all kinds of situations in which the code might end up. TDD is not a golden charm, but its strong points have been proven many times.

Can the code be improved?

Less code means better code. Most of the time, and excluding some particular cases. Think about:

• existing alternative functions
• different approaches that can solve problems in a better way
• how to reduce the code in the PR keeping the functionality
• reducing the cognitive complexity
• rearranging/restructuring the code
• better names
  Of course, the list is incomplete.

Do any of the added comments might make someone feel uncomfortable?

If there’s even the tiniest doubt, then rewrite the comment. Put effort into writing good-quality neutral suggestions, don’t let your emotions interfere, and put aside personal preferences so your day will go as pleasant as possible. Often times people have endless discussions on minor issues because of subjective interpretations and feelings.

Are your comments’ ideas clearly stated and backed up by arguments?

When people write the feedback, sometimes they do it in an alert state, trying to express a simple (or complex) idea that, in the end, has no meaning or is ambiguous for the pull request’s author (not even for other team members). Review your comments and try to put yourself in other people’s shoes. Invest time writing good feedback, or you will spend a lot more explaining it again, or, as it happens in practice, the suggestion will be implemented in a way nobody wants. Remember to always back up your ideas with solid arguments, be bold and recognize the cons of your suggestions.

Conclusion

While this checklist is not exhaustive, it contains a few ideas that will positively impact the quality of a codebase. Those are some lessons I learned along the way from others, experimenting or hitting the smallest finger of the right foot in the corner of the bed.