We are told that we should organize code reviews because code reviews good for our code base. We have followed this advice and managed to build a magnificent facade. We are doing code reviews and improving our code base. Everything is looking great from the outside and it might be true that we are making some progress.
However, we are not unleashing the full potential of code reviews yet.
Code Reviews Are a Waste of Time
The traditional code reviews have two major problems:
- The code base is too big. Because a traditional review is typically organized before or after a milestone, the reviewed code base is so big than nobody can read it through in a reasonable amount of time. Also, if we are honest, we have to admit that no one wants to spend a lot of time for reading through large chunks of code even if one would be allowed to so (and this not often the case). This is why the majority of comments are pretty much worthless.
- The feedback is always late. When a code review is not a continuous process that is integrated into our day to day work, a lot of code has been written before the code base is reviewed. In other words, if we find something critical from the code, the odds are that the same mistake is made more than once. We are screwed. We have critical problems in our code base and often we are not given enough time to fix them.
Traditional code reviews are a waste of time. It makes absolutely no sense to drag the whole team in a meeting room to discuss about review notes that are both late and useless. It would be much more productive to let us improve our code base instead of dragging us into a meeting which sole purpose is to give an impression that we care about the quality of our code.
Luckily for us, all hope is not lost.
Form Follows Function
There is a saying which says that road to hell is paved with good intentions. The intention behind the traditional code reviews is good but the execution causes more problems than it solves. Instead of leaning on burdensome processes, we should aim to make the code review a part of our every day work.
We can use pair programming and review the contents of each commit. These techniques solve most of the problems of the traditional code review process because they offer flexibility and instant feedback that is nonexistent if we choose to follow the traditional path. However, I have been wondering if there is something else we can do.
There are two reasons why we are doing code reviews:
- We want to keep our code base clean and eliminate as much crappy code as possible.
- We want to share knowledge with your team members.
The five whys is a problem solving technique that is used to determine the root cause of a problem. Its main idea it to keep asking the question “Why” until the root cause of the problem is identified. So, what has this got to do with code reviews?
This technique helps us to identify the reason why the reviewed code is implemented the way it is. This information matters because it helps us to evaluate the current implementation against its intention. This helps us to concentrate on the function of the code instead of its form. To make things clear: form matters but it follows function; not the other way around.
As a bonus, we are spreading the hidden information to the other members of our team.
From Judgement to Improvement
The idea of using the five whys technique is probably not new for an experienced software developer. It is a technique that we should already be using in our daily work. However, it has one unexpected advantage.
Traditional code reviews can be unpleasant situations which can cause unnecessary friction between our team members. Some of us have the tendency to take all feedback personally and some us are less than great at giving constructive feedback. This is not professional but it is very human.
The five whys technique is a clever way to include the software developer who implemented the reviewed code to the review process as an active participant. This reduces stress because it makes our fellow developer feel that we are trying to understand his decisions instead of just judging them.
This is a huge benefit because now we can focus on our common goal and start making continuous improvements to our code base.
Mitigating the potential impact, that is wasting time, of a mammoth commit using reviewed incremental commits as a part of whatever overarching process one may follow is most likely the best course of action, but this is just my opinion.
I agree with the sentiment expressed in your post, and have been the reviewer of mammoth commits, in cases weeks worth of work, with my mind unable to cope with the load, probably thrashing. In such circumstances, it places pressure on the reviewer to just nod, since the deadline for the work may already have passed. In these cases however, it was a one-on-one review, so more time efficient than a multi-person meeting. Would I say it was a waste of time? No, since it is better than nothing and above all is a whole lot better than a culture where individual developers go off on a tangent making drastic changes at will without consulting anyone - I have seen this happen many times too.
thank you for your comment! Like you said, reviewing a lot of code is never totally worthless. It is definitely a better practice than not reviewing code at all. I have also done my share of this kind of work and noticed that it is really hard to concentrate and keep your focus.
Luckily for me, there was no organizational pressure and I could take the time that was required to do a good job. However, the problem was that there were some problems that were also found from the code that was written when I was reviewing the "old" code. That is why I started believing on constant code reviews (or incremental code reviews).
Of course this does not work if people have the tendency to do one large commit instead of several smaller ones but that is another problem. If you can get these people to change their behavior, you can also use your veto power sooner and minimize the "damage".
Ben Collins-Susman has written a great blog entry called Programmer Insecurity which talks about the benefits of frequent commits. I recommend that you read it and share the link to people who are constantly doing those mammoth commits.