Overkilling code reviews
Like all the components of software development processes should be a contextual decision and not “Because company X does it that way”!.
If you work in an agile team, code review is a common task you do every day. From changing a config for deployment to adding a new feature, there are new codes you need to review before those codes end up in the application. The review process is something which is different based on the team.:
- How many approvers are needed?
- Who can approve?
- What can every group approve?
- What checks need to be green before you can merge the PR?
A lot of teams spend a lot of time creating scripts, complicated Git config files, and different toolings to force these restrictions and policies. Like many other decisions in an engineering team, these policies and restrictions should be contextual to the size and state of your company. Maybe you don’t even need code reviews, maybe one approval is enough or maybe developers can merge, and reviews can happen even after it’s merged, and if there are different opinions, there can be follow-up tickets.
Agility is about contextual decisions, not following best practices in big organizations .
The code review should help increase the productivity of the team and not frustrate the team by overcomplicating the processes. Many teams think the code reviews must happen before the code gets merged, but in reality, the team can review the code even after its merged to provide any feedback or share knowledge.
The end goal of a code review is to merge the PR!
Many articles show how you can overcomplicate your team processes and waste your team's time on resolving merge conflicts and rebasing branches due to the delays caused by the established code review process. I want to encourage you it’s ok if you don’t follow that FAANG company process for code reviews. Look at your own situation and requirement to design a process which matches your need. The main pillar of my opinion around code review is you can trust engineers' judgment in many situations to simplify your processes and make your team more productive.
In this post, I talk about three pitfalls I saw in the small engineering teams I worked with.
They review everything
This might be controversial, and many think that reviewing everything is needed to avoid breaking anything in production.
I think if we look at it from a trust point of view, maybe in many startups and early-stage products, some scaffolding work is happening, or the consequences of bad merges are not that bad; in that case, maybe you can let changes get merged and then the team always can review the new changes and provide any feedback later on.
Adding tests, staging environments, and proper CI/CD processes might be more beneficial in the long term than trying to protect the app by forcing complicated code reviews.
Too many approvals needed
Another problem with an overkilling review process is too many approvals are needed. For example, let’s say in a small team of 6 engineers adding an approval number of 5 is too much! Again think about what’s the purpose of the code review? Learning about the change? Provide some feedback? Or protect the app from bad merges?
For most small teams having one approval is enough and the rest of the team can spend reviewing the code after it’s merged to learn about the changes and provide any further feedback.
If the purpose of a PR is merging the new changes, are all the steps you added to your code review process really necessary?
Complicated automated checks
On many CI/CD platforms like GitHub, you have the options to turn on the checks for automated tests and checks before the PR gets merged. For example, if the unit or performance tests fail, GitHub blocks the branch from getting merged. This is another area where you can rethink and ask what are the consequences of a one failing test? If you have a staging/test environment, isn’t it safe enough for the PR to get merged, and the author does a follow-up to fix the failing tests or even revert the PR if needed?
The reminder here is you need to build your processes based on your needs and not what makes them less error-prone. Sometimes the cost of protecting your codebase from any errors is higher than the cost of the error itself!
Final thoughts
Something I suggest to the companies I work with as my clients is to start using the least protective rules in the beginning and add new rules as you grow and learn more about what you really need.
I help companies build their products and their teams. If you have any thoughts on what I mentioned in this blog post, you can leave them here in the comments or reach out to me by email or on Twitter.
If you enjoy building products as a freelancer developer and you’d like to know more about the projects I’m working on, you can connectwithmostafa@gmail.com
