Disclaimer: I recognize the fact that in some industry domains code review is a mandatory part of the process and a company would not be able to get a product certified or sold without a proven and rigid code review process. During my time with Motorola Solutions, I worked on Public Safety systems, so I this kind of experience - the same applies to any kind of software where risk to human life or high financial loss is a factor: controlling of energy plants, transportation, space missions, medical equipment. The story that I'm about to tell you is *NOT* about any of these. It is about the regular software development that is done by 80% of us, software developers, all over the world.
The stone age - code inspections
I remember the times before anybody used Scrum (or anything similar) when code was subject to formal review process. At the time, it felt as something important, as one more way of making a good quality product. It needs to be added that when I first started doing formal code reviews, the code being reviewed didn't have any Unit or Component level tests. Probably because of this, everybody felt it was important to do the reviews.
Experiments in the Scrum era
As soon as I learned how to do a decent Scrum, I started seeing code review as something that slows us down. A developer finishes the coding, moves the thing to the In Review column on a board and then it starts. Two or three other colleagues need to look at the code and put their comments in. And no, they are not sitting idle and waiting eagerly for anything to be reviewed. They are in the same situation - they have their own things they are working on and their own things being reviewed. This often led to delays in reviewing anything. Anybody who moved their item to the In Review column had to wait at least a couple of days to get any feedback.
It was perfectly natural behavior to seek for something new to work on, while waiting for the current review to finish. So, the developer took a new item, started working on it and then when the review comments for the previous item were in place, the developer suspended the current work for a while to resolve the comments and get the previous thing really done. I didn't like it, I can tell you. I thought: why we are doing this? This whole review process is a manifestation of two of the Lean Software Development wastes: handoffs and delays and in the end it also leads to the third one - context switching.
Initially, of course, we started encouraging the teams to start the review quicker. Anything must not stay idle in the In Review column - when you see anything in there, don't postpone it, suspend what you are doing and do the review in a timely manner. It was a good thing. The other problem that I noticed was that usually, in any team, there was someone very important called The Architect and this person was mandatory on all reviews. Obviously, if you are a very important person, you don't have the time, so this person was typically late with all the reviews. That was when I introduced another helpful rule called The Train is Leaving. In detail, the rule said: as a developer, I'm asking you to review my code. It is not only in my interest to have it reviewed, but also in yours to review it. If you do not have the time, the review will not wait for you. The reviewers are supposed to start the review immediately and even if you are a VIP, you only have so much time (e.g. 2 days) to put in your comments and if you don't we will merge the changes without your consent - The Train is Leaving. It was probably at that time when a mutual dislike appeared between me and the architects and it has never gone away.
Another good thing that we started promoting was a "review half-way through" which you could say was a degenerated form of pair-programming. Instead of asking people to look at a completed piece of code, a developer would ask one person to look at the code when it was still incomplete, but there was already enough code so that the other person could grasp the main idea. Sometimes it changed the course with regard to coding conventions, common components, etc. Then, the regular (but improved, The Train is Leaving, etc.) review took place when the coding was completed.
The situation was better than initially, but I felt it was still not what it needs to be. I had a team at that time where I decided to do a more extreme experiment: remove the review altogether.
The fear of being without it
The single word to describe how the team reacted to this change was: fear. It was an eye-opening experience, because we learned why the review process was important to developers in the team. I think I can summarize it a couple of points:
- Safety net - I know that whatever I implement, it is going to be reviewed by my colleagues. Even if do something silly, others will look at my code and will have a chance to notice it. As soon as I'm deprived of this safety net, I fear of merging my changes, because now I really need to be sure that what I'm merging is a solid, well-written and maintainable piece of code.
- Less knowledge about the changing codebase - with the review process in place, I knew how our codebase changes. Now, people are making changes I'm unaware of.
What was really important was the fact that the developers did not actually complain about the chances of finding bugs being lower. The myth of bug discovery being the goal of the review process fell apart. We learned that we need to be more disciplined when it comes to testing and obeying coding conventions. At that time, we already had much better testing harnesses, esp. including Unit Tests. So, the way for a developer to combat the fear of the safety net not being there was alleviated by testing. It is through the testing (all types, all levels) that we find bugs and it is through the regression testing that we build a safety net for not breaking things that have already been built. The discomfort of being alone while merging the code was still around, but it abated.
We were also able to maintain a decent knowledge about the codebase by doing occasional code walkthroughs. At the walkthroughs, we did not focus on individual commits or merges, but rather on wider, product level changes, such as "this is how feature A was implemented in the code" or "today we will look at the changes related to feature B, which is half-way to be completed".
I have never done anything similar with any other team (more on that later) but the findings from the experiment shaped how I approach the review process to this day.
The onset of tools for code review
In the next years, the tools that we started using for backlog and spring management (JIRA, Azure Boards, etc.) started promoting the idea of code review by either supporting it off-the-shelf or by adding integration to code reivew tools like Gerrit. I realized that whether I like it or not, the review process will be there for good.
I have never did another "no review" experiment, but I started encouraging the different teams I worked with to approach the review in a nimble and productive way:
- let's be honest: code review is not there to find bugs, even though many people in the world are believe otherwise
- the major thing we are getting from the review is the knowledge about the changing codebase
- let's strive to get any item through the review really quickly
- if you have something really simple and small, don't invite too many people to review it - maybe one will do
- if you have something small and it is really urgent, merge it without the review; you will ask you colleagues to review it tomorrow, but it has to get in today.
- for God's sake, don't ever configure your review tool to disallow merging the code without approvals!
Obviously, the teams where I try to implement this healthy approach to code review must already have a really good test harnesses, at least consisting of UT and functional tests, ideally also of component level tests. This is how we find bugs and this is how we prevent ourselves from breaking things that already work correctly. If you have a bug that escaped your tests, there is only a very remote possibility that it could have been caught at a code review. In this case, you need to get better at testing, not better at looking at the static block of text.
Heated discussions nowadays
The discussions on the topic of code reviews are often very heated. Some people present an approach close to mine, some truly believe that the review is there for finding bugs. I was briefly engaged into one these discussion on LinkedIn and I remember a gentleman saying something like:
"I have been working in the software industry for 25 years. With all my professional experience, I would never want my code to go to production, without anybody looking at it. Even with all these years of experience, I don't trust myself enough to do that, because there may always be something that I did not notice."
These kind of statements really turn me on and a response that immediately comes to me is:
I don't trust myself either, not enough to integrate a change straight to production. But I build my confidence by testing my change and by letting others test it. Definitely not by letting others look at a static block of text. If the review is what you need to deploy things into production, then I'm sorry for you and I wouldn't like to use the software that you developed.
Please also note this important fact: many of the major software products in the Open Source domain (like git, PHP, Linux kernel, Python, JavaScript, CouchDB, TeX, Matplotlib and many others) were initially developed by one person. This person did not have anybody else to review their code. But they pulled it off and the built a great product. It was only at a later time, when the product was opened for contributors around the world to work in it, that the review process was introduced.
Conclusion
After what I have experienced with different software teams, I'm completely convinced that the key benefit of any form of code review is staying up to date with the changing codebase. Finding bugs should not be considered a benefit of a code review - the bugs are found much more efficiently through the different stages of testing. That a work item has been tested is usually part of a DoD (it should be), so any bugs that we find through testing will be found before the item is Done.
The fear of being left alone with a merge decision is completely natural and even for the sake of mental comfort of the developers, the review process is a good thing. And anyway, the support of the review process in all modern development tools means it will be there for the years to come.
For the sake of smooth delivery and short cycle time, it is important that the approach to the review process is lightweight. Scrum Masters or other coaches should work with the development teams to establish a culture where code review is a quick, supportive process and where it does not take the attention away from the other parts of the development process, esp. from testing, whose impact on product delivery is far more greater.