Pre-commit code review can provide some tremendous benefits: It improves quality of the final code, it helps team members to learn from each other. It also propagates knowledge about projects and best practices. But it also generates additional problems. Here's how we dealt with them.
Pre-Commit Code Review at Evojam
In short - every code commit needs to be verified by an automatic code-checker and then by two developers independently. We use Gerrit for peer code review.
So this is how the perfect world looks like.
Reality is messier than that ;)
Problems
Over the last few months problems with code review have piled up and started affecting our work. The team has come together to deal with them in 3 company-wide meetings. We aimed to identify problems, find root causes and brainstorm solutions.
On the first meeting we've build a list of problems:
Asking for review - asking for code by interrupting others' work. Those are bad practices of either walking over to someone and asking for review or pinging them directly on Slack.
Commits stacking / Fixes in next commit - when someone submits a number of commits regarding the same functionality and they're not self-contained. In Gerrit the basic unit of work is a change. In the beginning it equals to a commit, but it can have versions as the code is being improved upon. There's no pull-requests. It's the commit that gets reviewed. So each commit needs to be complete and can't break the application. If it does, it will be rejected.
No info about commits waiting - some commits have been waiting for a code review even when there was someone who could do it but they didn't know about it.
Time consuming - This is the part where we consciously decided we do want to spend time doing code review as it saves a ton of time in the long run. But we don't want to spend any more time than absolutely necessary.
Cross-project review - Over the recent period of time our team consisted of around a dozen of developers working on 3 projects. Some of them specialize in frontend, some of them in backend. So there had been a need for people from outside of the current project to review changes in a different one. This meant reviewing changes in a project that a given developer knows little about. This kind of review still works but provides less help as the knowledge of an outside developer ends on the language and formal stuff. Moreover different priorities and pressures in different teams may mean difficulties to find time to do a review in another project.
Not enough automatic checks - there's plenty of things that can be verified automatically before even someone sits down to do a code review. Things like order of imports, attributes or code conventions. We've used some of those as part of the automatic code review triggered by Jenkins, but there were some aspects that could have been automated even further.
Comment quality - sometimes comments to a commit or to review were very short, to the extent of not being understandable.
Turbo (big) commits - this is a large commit that accumulates code changes from days or even weeks of work. It can be very hard to review those changes.
Missing +1 on rebase - sometimes we'd loose the first review on a commit when doing a code rebase (remember, each change requires approval from 2 developers). Fixing this required addition of a Gerrit plugin that prevented +1 approvals from disappearing.
Finding Root Causes
Second meeting was devoted to uncovering the underlying causes. I can honestly admit that this was the most frustrating meeting. The team was strongly affected by the problems and solutions didn't appear to be in sight.
We did experiment with 5 Whys, we dove deeply into the most pressing problems. Moreover there was no clear idea on how to go forward. The outcome of this meeting was a chaotic list of root causes and clumsy solutions. Nobody seemed to be happy with the results.
Solutions & Recommendations
For the third meeting we've decided to take on a different, more organized approach. We've had a refined list of problems and their root causes prior to the meeting. We went through the list and everyone has written down their ideas. Each on a separate piece of paper.
We've then gathered them all in one pile and went through it one-by-one. Naturally good ideas have appeared on multiple cards and they've been clustered together. Below's the list of ideas we've implemented and ones we decided to park for later.
Adopted Solutions and Practices
Limit cross-project code review - This turned out to be a root cause of many problems. It lowered the quality of code review, created a need for interruptions in other's work, etc. We've decided to organize our work in such a way that a cross-project code review is not necessary.
Teamwork organization - When team works together for a longer period of time they learn their habits and work out ways of non-blocking, asynchronous code review.
Sketch & Fill - This is a solution for big commits of new features. Design your new solution or feature first. Create stubs of objects and methods and commit that. This way the new solution is clear and readable to the reviewers. This is also a great opportunity for the feature developer to plan it ahead more thoughtfully. We do it by implementing the skeleton of the proposed solution and putting TODO or FIXME in the code.
Daily update - In case of any code-review related problems we explicitly decided to raise them on the daily scrum if necessary.
Start with code review - Whatever your reason for leaving the computer was, when coming back start with code review, or otherwise incorporate code review into your team's workflow. You can also use mental triggers - each time after lunch, break, coffee, foosball, etc.
Know Git. It's really important to know Git very well first, before you can fully benefit from Gerrit flow of work. You need to have full control over the code, local branches, rebasing, etc. Using Git from the command-line is strongly recommended.
Configure Gerrit Properly Problems with +1s missing during rebase were due to Gerrit misconfiguration. As well as some performance issues. We've also configured default reviewers groups in Gerrit so everyone on the same projects sees exactly what commits are waiting for review.
Online commit modifications Newer version of Gerrit allows for inline commit edits on the web, so we've upgraded. Improving the code from the web level is the quickest solution for trivial changes, like misspells.
Provide additional comment. If the code is not self-explanatory then an extra information needs to be put into the comment. Like TODOs or planned changes or intention behind the code.
The truth is that nothing will replace effective team communication about the need for code review. It needs to be non-interruptive, asynchronous and clear.
Postponed Solutions
Gerrit topics. There’s a Gerrit feature allowing to group commits with one topic. To be checked in future.
Finish current feature first. Don’t start new feature until all your code commits have been reviewed. This is a radical implementation of the idea of slack time in Agile: never begin new feature until the current one is fully finished and ready for production. This causes some problems as single feature may be spread across multiple commits, etc.
Slack bot + gerrit integration. We may try that out in future but only if it's well configured.
Bad Ideas
Yup, we've had them too. In the process of discussion and sometimes trial&error we've rejected them.
Slack #review channel. Nobody would like to look there probably. It would also ignore the root cause of cross-project code review.
Commits with priority level. Would require additional work on both submitting and receiving end of the review process. Plus all the mess with actually deciding on priorities. And the show must go on - all the code needs to be reviewed eventually.
Bring crisps to reviewer. Nope, unhealthy. And interrupting.
A Superman. Each week assign one developer to do all the code reviews. This would mean that this person is partially excluded from a current sprint. Creates a bottle neck, limits knowledge propagation and ignores the root cause of poor team communication.
Conclusion
There's no ideal solutions for code review. Especially in a dynamic setup when there's a lot of things happening in projects everyday and different people bringing different perspectives.
One of the takeaways is that conducting such large team meetings requires a lot of wise moderation from the leaders. Otherwise those meetings aren't very productive and can make people frustrated.
We've managed to improve the process a lot and learned a ton on the way. We believe it's worth investing in pre-commit code review and hope that the thoughts above will help in your implementation.