Code Review

One of the most important practices every engineering team should have

After years of working with software development, I came to realize the benefits of having a code review process on your team go far beyond just catching bugs and prevent issues in production.

Code reviews are perfect for ensuring your team is in sync regarding code style, standards, patterns, and best practices.

When the code review process is fluent and truly works, the entire team share and advocate for best practices.

Every team is different, and code reviews are the common language where developers can speak equally independently of their levels.

Table of contents

1. How it benefits developers

2. General rules

3. The process of doing a code review

4. Code Review strategies for teams

—

How it benefits developers

Code review for Jr developers

Code review is not a practice restricted to Senior developers.

Junior Developers have a lot to learn by doing code reviews on other developer’s code. The code review practice is a moment when people share knowledge and best practices.

If you are a Junior Developers, I recommend spending at least 40% of your time reviewing other people’s work.

Use this moment to learn as much as you can. Code Review is the best way to understand how your teammates make changes to the system. 

By looking at code produced by teammates, you will learn about patterns, best practices, dos, and don’ts.

Don't worry about understanding the whole context right now. Use this moment to ask questions and learn about the dynamics of pushing new code to production.

If there is some new technique you never saw, ask for more details. If the code is hard to understand, say it too! Programmers should write code that is easy for people to read, no matter the experience.

Code review for Mid-level developers

For mid-level developers, I recommend spending at least 20% of your day doing code reviews.

Remember to be kind when receiving feedback. At this level, developers usually deliver many tasks that will be reviewed for both Junior and Senior developers.

It's also essential to give helpful feedback to other developers.

If there is something you disagree with, explain why you think it should be different. Most of the problems can be solved with different solutions.

Code review for Senior developers

If you are a senior developer, it is your job to ensure everyone follows the team standards and best practices. It turns out, doing a code review is the best way to do this.

I recommend spending at least 30% of your time doing code reviews.

Try focusing on finding things that don't fit the project code style or some unknown patterns. It's always good to provide external sources (links) for further research when recommending some changes.

For example, you see a change that may benefit from an Observer pattern. Instead of complaining about the current implementation, explain the benefits of using the patterns, and provide resources so the developer can research and implement the changes themselves.

Code reviews are also a great place to set the bar high for Junior and Mid-level developers. This is the perfect moment for people to get out of their comfort zone by absorbing feedback from a more experienced colleague and learning during the process.

It's also your job to make sure code reviews are not in freeze mode. 

Sometimes people don't do code reviews because they're focused on their current tasks.

This is a problem since we expect that every code that goes to production passed through the code review process. When this happens you can see a Tsunami effect at the end of the cycle where everyone asks for code review as fast as possible, otherwise, they won't deliver what they promised.

So pay attention not only to the quality of the code reviews but the code review process itself.

—

General rules

Like any other process or methodology, code review have its best practices and some non-written rules.

Be Kind
The person you're doing a code review is a teammate of yours. You work together, and there is no reason to leave the Ego mess the team relationship here. If you're doing a code review or responding to a review, be kind.

Ask for Reviews
It's okay to ask for reviews.

Don't expect people to proactive review your code as soon as you finish an implementation. People have other things to do, and they can't afford to watch every move of their teammates to know when there is something new to review. If you finished something ready for review, ask for it!

Do code reviews
If your team uses a channel or a kanban column to indicate a new code that needs review, find some time to do it! It does not need to be right away — (except for hotfixes) — so plan time to focus on code reviews. I recommend reviewing the code daily.

—

The process of doing a code review

The first tip is to have dedicated time to do a Code Review.

If your company is a heavy user of a calendar and your team holds lots of meetings, I recommend scheduling an event for yourself so no one can interrupt you during the code review.

I prefer doing small chunks of code reviews. Try not to review the code for more than 1 hour.

A Cisco Systems programming team study revealed that a review of 200–400 LOC over 60 to 90 minutes should yield a 70–90% defect discovery. Source

So keep in mind that big pull requests should be reviewed at separate times so they can get the right amount of attention from you.

If a routine works for you, I recommend scheduling a fixed time every day to do code reviews. I prefer doing them after lunch so I can start with a fresh mind.

Make sure you know what you are reviewing.

Before you start taking a look at the code, make sure you know what you are reviewing.

Is it a new feature that enables users to update their profiles? Is it a bug fix? Or is it refactoring, so you need to ensure the application behavior remains the same?

I usually ask myself the following questions before starting.

1. Does the PR have the necessary tests to cover the change?

2. Does this change require integration tests?

3. Does the documentation related to this change has been updated? For example, the readme or a Wiki page.

4. Does this change cover the product requirements? (Ex: Validations for forms, interface matching the layout, usability and accessibility needs, etc.)

5. Does this change update any library or dependency that may break the system?

6. Is there any extra task the developer needs to do after merging this code? (Ex: Running a migration, updating a different repository, etc..)

If any of these items are checked before starting the Code Review, I usually ask for more information.

Test the changes before digging into the code

Imagine finishing a review, approving it, and after that, someone notices that the code is not working.

This may sound silly, but it happens a lot.

Approving a pull request without even testing the code it’s very dangerous, no matter the size of the pull request — some 2~3 line changes may not need to be tested, but those are the exception, not the rule.

There are many solutions to the same problem.

Keep in mind that most of the problems have different solutions. Make sure you are not trying to force your peers to think the same way you do.

Instead, acknowledged that your solution is not the only solution, and if you think the proposed code can be better, be kind and explain why you think it’s better.

It's common to see developers trying to enforcing their code styles among other developers’ code. This is wrong.

If there is a code style for the project, everyone should follow it, and however, if it’s something that it's not on the code style document, people should be free to code the way they think it’s best.

Check the commits

Commits are responsible for telling the story behind the changes of a codebase. If the commits are a bit vague, you may ask for the developer to improve them.

# BAD Fix bug  # Good Add a missing Int type to fn Run() to fix the build

Look at Conventional Commits for best practices.

Leave comments and suggestions, not complaints.

Reviews should be concise and written in a neutral language. Critique the code, not the author.

Avoid things like “my code worked before your change”, “your method has a bug”, etc.

If necessary, add links to docs or specifications to explain your point. When something is unclear, ask for clarification rather than assuming ignorance.

When you're done, make sure to explain the impact of your review.

If there are only suggestions to improve the overall code but may not impact behavior or functionality, make sure to clarify that, for example: “Feel free to merge after responding or accepting some of the suggestions. The suggestions are not a blocker to merge.”

When a critical observation needs to be addressed, explains it too: “Thanks for the implementation! I noticed that we changed behavior that may break Feature X. Please consider reviewing and fixing the issue before the merge. I left some suggestions but feel free to contact me if you need a further explanation.”

—

Code review strategies for teams

It's not hard to see teams that tried code review a couple of times and complained about not finding a proper way to execute the process.

Sometimes the code review column on kanban has too many items or code reviews from peers are too light (LGTM for everything).

To prevent such problems, I recommend using a strategy that fits your team. Here are some examples.

Work in pairs (not pair programming)

Imagine there is a User Story that requires the creation of a search engine for e-commerce. It is easy to identify that multiple parts of the system should be modified to implement this task. It requires someone to code the search input on the frontend of the site so the user can type what they are looking for and submit the request, and there is also the backend code to process the input and query the database.

In this example, two developers can work in parallel, executing both tasks, coding the frontend, and coding the backend. As both developers are on the same User Story, every time one developer finishes up a task from that story, it should review the code from the other developers they are working with.

This system works if your team is full-stack (developers can write and review frontend and backend code)

The benefit here is that the person reviewing your code also depends on it to finish his job, so it’s natural to see developers spending more time doing a better code review.

One Task, One Review

This strategy is helpful for large teams that do a good job breaking down tasks into small chunks of code (See Break Down large pull requests into smaller ones)

The process is straightforward: Before starting a new task, you do a code review for each task you finish.

If the team produces lots of small pull requests and this strategy works, the frequency of code going to production should also increase.

Daily reviews

This strategy is also nice for large teams or teams that work on big tasks. It requires more responsibility from the developer because it’s a daily habit.

On this strategy, everyone agrees that they will do morning code reviews, so every day at 9 am (or first thing in the morning), the developer will look for code waiting for review.

As this is a daily task, it’s nice to schedule a one-hour “meeting” to block everyone’s calendar so you can make sure developers won't need to skip this practice because of other meetings.

—

I hope these tips can help you and your team to implement or improve the code review process.

Feel free to leave feedback or email me at hugoodias@gmail.com

See you!