One of the most important practices every engineering team should have
|Hugo Dias||Aug 1, 2020||2|
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 than just catching bugs and prevent issues in production.
Code reviews are perfect to make sure 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
How it benefits developers
The process of doing a code review
Code Review strategies for teams
How it benefits developers
Code review for Jr developers
Different from what many people think, code review is not a practice restricted to experienced developers (Senior).
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. In this level, developers usually deliver a high number of tasks that will be reviewed for both Junior and Senior developers.
It's also important to give useful feedback to other developers.
If there is something you don't agree with, explain why you think it should be different. Most of the problems can be solved with solutions.
Code review for Senior developers
If you are a senior developer, it is your job to make sure everyone is following the team standards and best practices. It turns out, doing a code review is the best place 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 a known pattern, 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 a freeze mode.
Sometimes people don't do code reviews because they're focused on their current tasks. It's okay but, too much pull requests waiting for code reviews can block the continuo
Like any other process or methodology, code review have its best practices and some non-written rules.
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 for every move of their teammates to know when there is something new to review. If you finished something that is ready for review, go 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 — (expect for hotfixes) — so plan a time to focus on code reviews. I recommend reviewing the code on a daily basis.
The process of doing a code review
The first tip is having a 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.
Prefer doing small chunks of code reviews. Try not to review code for more than 1 hour.
A study of a Cisco Systems programming team 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 huge pull requests should be reviewed in 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 are you reviewing
Before you actually start taking a look at the code, make sure you know what are you reviewing.
Is it a new feature that enables users to update their profiles? Is it a bug fix? Or is it a refactoring so you need to make sure the application behavior should remain the same?
I usually ask myself the following questions before starting.
Does this change have the necessary tests to cover the change?
Does this change need integration tests?
Does the documentation related to this change has been updated? For example the readme or a Wiki page
Does this change cover the product requirements? (Ex: Validations for forms, interface matching the layout, usability and accessibility requirements, etc)
Does this change update any library or dependency that may break the system?
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 I start doing 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 actually 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 — there are some 2~3 line changes that 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 its 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, however, if its something that it's not on the code style document, people should be free to code the way they think its 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 complains
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 yo explain the impact if your review. If there are only suggestions to improve the overall code but may not impact behavior or functionality, make sure to explain that, for example: “Feel free to merge after responding or accepting some of the suggestions. The suggestions are not a blocker to merge”
When there is a critical observation that needs to be addressed, explains it too: “Thanks for the implementation! I noticed that we changed behavior that may break the 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, sometimes 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.
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 useful 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 very simple: For each task you finish, before starting a new task you do a code review.
If the team produces lots of small pull requests and this strategy is working, the frequency of code going to production should also increase.
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 1h “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 email@example.com