How to perform effective code reviews like a human being
Code reviews main goal is to improve our solutions while learning new technologies and techniques. Implement an effective and efficient code review process is not easy. It’s important to not only consider them as a set of tasks and principles, but as a major behavioural change operated through task and principles.
Why Code Reviews?
As with any intellectual work, software has intrinsic inclination towards failure. Code reviews are a way to mitigate failure by:
- increasing code quality
- ensuring coding standards
- validating the implementation
- sharing knowledge on the areas being changed
What to review?
It’s important to understand that CRs are not a sole responsibility of the reviewers. Rather, they are the result of the collaboration between two or more actors: reviewer and author.
Code author
As code author you have to make sure your code is ready to be reviewed by providing all the necessary information.
Make the change easy, make the easy change
The scope of the change should be small and the boundaries need to be clearly defined. Big changes are difficult to review, highly error prone and hardly mitigate the risk of deploying faulty software. One way to mitigate this issue it’s to break the changes in smaller chunks. You will deliver more frequently and reduce the risk of each deploy.
What is the intent of the change?
Provide a description of what is the purpose of your change and why you opted for that solution ( you can add a link to the task if you are using task management tools like JIRA ). In case there are UI/UX changes it may help to add a before/after screenshot or a gif to show what changed.
Define clearly the intent doesn’t only facilitate the reviewers work. It provides a safer context to the reviewer, reducing possible frustrations and impacting positively the consequent interaction along with the quality of the review.
Is it ready to review?
It may seem trivial, but make sure to review your code before you ask someone else to do it. Run the test, wait the CI to finish, test it manually if it’s necessary. Ask yourself if you would be willing to review it if it was someone else code. Once you feel you PR is ready is your responsibility to make sure your PR is reviewed.
Code Reviewer
Code reviewers are not gate keepers, nor police officers. They have 2 main responsibilities: help and learn.
Help: You provide your support in order to improve the proposed changes. Help your colleagues to come up with a better solution, to learn new techniques and to be better at their job.
Learn: While reviewing other people code, you learn how the product works, you learn about different approaches, you can improve your existing knowledges by explaining them. Code review is a game where everybody win.
What do you want to review?
It’s extremely important to define what needs to be reviewed. Most of the time the list of topics to review will look like the following:
- Code style
- Security
- Reliability
- Purpose
- Performances
- Implementation & Maintainability
Every point in the list is super reasonable. However I strongly believe the more the reviewer has to review the higher is the risk of mistakes and conflicts between the actors.
It’s important that we switch our focus on what requires human intervention and automate everything else.
Code style: Tools like ESLint or RuboCop are a must have. Don’t waste time and energy discussing about where to place your curly brackets. PRs are great to raise the need of discussing a rule, however they are not the right context to address it. Code style rules need to be defined in separate sessions, then enforced it via automation.
Security: Security requirements need to be defined before you start to code, it’s really hard to “generally” review security. Then you can integrate SAST (Static Application Security Testing) in your CI using tools like NodeJsScan, Brakeman, Bandit etc...
Reliability: It’s extremely important to ensure the code we are going to deploy runs properly and it is back-compatible with the current functionality. “Work on my machine” is really not an option we want to endorse!
Purpose: Writing code is one of the most expensive way to solve a problem. What and why has to be defined clearly before you even start to think to write code. If during code review you have to question the purpose of a change, then probably you have a bigger disfunction to solve way up your development pipeline.
Performances: As most of the previous points we can automate performance test, but they are pretty expensive. Add a benchmark , e.g. if you are introducing a new query, it’s a great way to trigger discussion based on facts, rather than opinions.
Implementation and maintainability: In these cases we can mitigate the risk by implementing a strong culture of pair programming and pair reviewing. Doing that will transforms code reviews from a gatekeeping practice to an enabling one.
An exercise in human relationships
As Kent Beck pointed out during his talk at RailsConf 2020
Software design is an exercise in human relationship
We know it’s really hard to provide constructive help if you do it in a confrontational way or you don’t provide an alternative to the problem you raised.
Yet, many code reviews suffer from endless confrontational discussions, and lead to frustrations and resentments on both sides. As a result relationships degrade which lead to more discussions and so on. How can we try to mitigate this issue in the context of code reviews?
- Everything that can be automated has to be automated. If feedbacks come from a tool, there is no-one to fight.
- Set up clear and defined context in your pr. Everybody would start the review being grateful and not resentful for lack of clarity.
- Provide a guidelines and effective communication training for all the engineers. Nowadays soft skills are fundamental, without them you can’t work efficiently in a team.
- Promote face-to-face interaction over written exchange for resolution of conflicts.
- Increase awareness by submitting surveys on regular basis to have “NPS” for code reviews.
Of course these are only some options you can introduce and most of the measure you could implement are driven by contextual factors like company culture, diversity, communication channels, etc…
There isn’t a recipe that fits all the purposes. You need to work on the principles, observe, adapt and remember, at the end it’s all about human relationships.
