This post is about how we do manual code reviews at Staffbase and what benefits we see there
At Staffbase we have almost 200 software engineers. We create, change or delete a lot of code on a daily basis. And each change in code has to follow our pull-request process which means a lot of automated checks.
Some checks are conducted automatically: coding style, test coverage, static code analysis, check for code vulnerabilities … But, after all, code reviews at Staffbase are real peer reviews. So, each pull request has to be checked by one or more teams, depending on the code ownership of the changes.
Really! We execute manual code reviews in an engineering organization consisting of 200 engineers. This slows us down and speeds us up at the same time. Let me explain what is meant by this contradiction.
In the short term and with focus on one particular pull request, this process slows us down. It would be much faster to just run automated checks and deploy the change automatically to production. Automatic deployment could probably be done within a couple of seconds. On the other hand, doing automated deployment might introduce bugs, vulnerabilities and failures that immediately hit our customers. Automatic deployment of pull requests is probably feasible for startups in their very early stages. Since our organization has to take care of the availability of a platform and the user experience of millions of users we have other stakes than automatic deployment of pull requests.
An organization not conducting code reviews would make fast progress in early stages of their existence but would make slow progress later.
In the long term and with focus on our whole code base, on our engineering organization, on the business value of our platform and customer benefit manual code reviews speeds us up a lot.
- Manual code reviews help us to find mistakes early. From our experience this is not the goal of automated tests to find bugs in new code. We are keen on high test coverage, but for regression testing.
- Manual reviews help us to have an eye on performance from the very beginning. By the way we write code we can derive whether it's fast or whether there is probably an index missing. Remember: There are millions of users on our platform. A degradation of performance will hit them immediately.
- The design of our code gets more consistent when another pair of eyes is having a look on it. In the long run consistent code can be maintained easier and the risk of introducing new bugs might be reduced at the same time.
- Conducting a code review is a great chance to share knowledge between team mates or between teams. In the long run this leads to shared code ownership and fosters collaboration in the team and beyond team borders.
Over the course of several years we’ve been able to measure, that the health of our codebase has improved.
There are a lot of dimensions of a code review that can’t easily be done completely automatically. When conducting a manual code review we have a lock on e.g.:
- How well is the code designed?
- Can the code be easily read? Does the naming match our rules?
- How easy can the code be understood by another developer?
- Do our automated tests check for the right (functional) things?
- Are there any vulnerabilities exposed not covered by the automated checks?
- Are potential performance issues covered in the change?
- Is the (API) documentation appropriate?
What we don’t really focus on in our reviews is code comments.
While being convinced that having manual code reviews is a good thing we face some issues that are not easy to solve. On the other hand these issues are only expressions of deeper issues that come to light during code reviews.
Each single piece of code has a team owning it. That’s a cool thing since we know who is responsible for a particular piece of code. But, we’ve a lot of dependencies in our code that span more than one team. These dependencies influence who is involved in a code review.
The root cause here is not the code review, but the code ownership across teams. We are working on that dependencies issue by improving modularization of our platform. But, this will take much more time. Therefore we have to cope with the status quo.
Sometimes getting a pull request approved takes a lot of time. Just imagine:
- the sprint is packed with stories to be solved
- planned slack time has already been spent on bugs and … code reviews
- there are still a lot of code reviews pending, some even from other teams
The moment a team spends capacity on getting code reviews done for another team this might endanger reaching sprint goals. Code reviews that are not handled in time might lead to unfinished stories for the other team.
The root cause here is again not the review itself. Basically, the slack planned in our sprints doesn't match the needed capacity.
Sometimes PRs get quite huge. For example you have to adapt some internal API* that is used widely in the whole platform. As a consequence, probably hundreds of changes have to be made in the whole codebase to not break the code. Reviewing the whole PR will be very time consuming and nobody planned for reviewing hundreds or even more changes.
* By the way, we change internal APIs only. Public APIs never change, but are published in a new version the moment they have to be enhanced.
Root cause here is also not the review but the code dependencies across teams mentioned above.
Nevertheless, we also discovered some best practices that help us to be more effective when doing our manual code reviews.
Sometimes it makes sense to approve a PR even if the code is not perfect but at least improves the code compared to the old version. This might help to speed up code reviews. The potential todos get written down in a backlog and are hopefully not forgotten.
By framing the goal of a code review that way we minimize discussions between reviewer and reviewee about which solution is right or best.
The smaller a pull request the smaller is likely the amount of time a manual code review consumes. Or, the smaller the PR the faster it might get reviewed. We are still working on how to reduce the size of PRs on average so it can be easier reviewed.
Don’t interrupt your current work to conduct a code review. In case there is a meeting planned or lunch break is approaching, it makes sense to use the time immediately after the meeting or the lunch break. That way the review is still in time and we don’t introduce additional context switches.
We don’t know whether we will get to a state where we stop conducting manual code reviews or whether we will reduce the number of manual code reviews in future. Right now it just feels right to do so while advancing our platform and having the user experience in mind all the time.
Code reviews help us to provide better and more consistent code, to find mistakes early, to have an eye on performance and to spread knowledge between team members. And, manual code reviews point us to some major issues we have in our platform - code dependencies, too little planned slack - and provide us with one more good reason to work on solving these issues.