As a developer and technology manager, I like to set really high standards to ensure that code is easy to read (understandable), simple enough for the task, and commented well enough that almost anybody could understand what is going on.
For a code review to impactful you need to know what you are looking for and what the desired outcome of the review is. There’s a lot of great reasons to do code reviews.
As a development manager, I like to do code reviews to reduce risk. Double checking everyone’s work is a great way to understand the quality of the work especially when the work has been done by a third party like a contractor or freelancer.
What is a code review?
A code review is having someone other than the author check someone else’s code for errors or mistakes. A really good effective code review will be systematic and basically follow a checklist looking to improve the code quality, locate errors, and identify any potential security problems.
The final result of a code review should be the changes being approved or rejected. If changes are rejected it should be very clear what needs to be changed, why they need to be changed and when the changes are due.
What should a manager use code reviews for?
A successful code review strategy requires balancing between the following: perfect code, high quality software, documented processes, collaborative environment and the egos of all involved.
As a technology manager, I tend to focus on the following things: security reviews, code reviews for following standards, and using code reviews to catch bugs.
Code reviews are an excellent way to share knowledge amongst the team. One of the main benefits I’ve seen as an individual contributor is that there’s usually somebody better than me that can teach me a new technique or algorithm.
When I work with outsourced or remote teams, I like to try and have an interaction every day with them so that I learn about hard parts of the code to work in or if I see inconsistent things.
As a manager it’s really important to try and stay positive during the code review because nobody likes to be criticized. Finding a bug before it potentially goes into production is a really positive thing, make sure you highlight the things that are being done correctly.
Where should code reviews be done?
Automated tools like linters, formatters and testing libraries are making code reviews a lot more effective because they can force the code to be more consistent and potentially easier to read. Integrating these tools while initially expensive can provide an immediate return on investment.
A good efficient code review should be done using the same process the team uses for creating the code, branching, and running automated tests. The reviewers time should be spent on reviewing things that are missed by machines.
Reviewing more than 1,000 lines of code will likely be a waste of time as it will get increasingly difficult to find errors. In the past, I’ve heard never to review more than 400 lines of code at once. I’m not sure where that number comes from, but I know that my eyes start to gloss over somewhere that arbitrary limit.
When should a code review be done?
Effective code reviews are done very frequently, at least for every major change request. As mentioned previously, they are an excellent source of feedback and a great way to share knowledge amongst the team.
Before creating a pull request
Before creating a pull request for someone else to review, you should review your own code. You should be really trying to answer these sorts of questions
- What was the purpose of the code change? Do the changes meet the purpose of the code change.
- Is this the best approach?
- Do the changes meet the norms/conventions of the existing code?
- Are variable and function names concise and descriptive?
When a developer is finished resolving an issue (whether it be a bug or backlog item) a code review should be done. The code review should normally be done by peer where they try to answer some really simple questions like: “Are there logical errors in the code?”, “Are all of the requirements met?”, “Are the automated tests testing the new code?”, “Does the changed code confirm to our style guidelines?”
After the Automated Tests Pass
If the automated tests are failing, there’s probably no need to do a code review because it’s likely causing problems. Doing it after the tests have passed means we are ensuring there’s stability.
After automated review
I believe that automation can really help increase my leverage which results in a lot less time being wasted. If there’s too many errors in the automated review, I generally won’t look at the code and will send it back to the developer for changes. Automation has a high potential to cut down on errors, save time and reduce cost.
Everyone’s code should be reviewed.
Regardless of how senior the staff member is, their code should be reviewed because it’s such an impactful way to learn and share amongst the team. If the reviews are clear, and positive there’s no need for anyone to have their ego bruised.
Keep it fact based
Make sure that everything said or done in a review is a fact and not an opinion. It’s really important that developers know why they are being asked to make the change instead of it being someone else’s personal preference.
I’ve come to realize over the years, that if I express an opinion it will sometimes be quietly implemented regardless of why I’ve suggested it. It’s much better to form everything to be fact based instead of expressing things like “This component should be stateless” – we should use something more like “Since the component doesn’t have any lifecycle methods and isn’t using any state we could make it stateless which will make it more readable and improve performance. Here is some documentation about how to improve it”.
In summary, it’s really important that we provide code reviews as part of the development process as it’s a really great way to learn from one another.