Is code review an integral part of a software development process?

[0]

"If two programmers like each other, they do pair programming, if they don't - they do code review"

tl;dr:

Code reviews are good for your project.

Let’s start out big:

Code reviews help to find more bugs than unit tests.

“- How is this even possible?!?” you’ll ask.
“- We already agreed that our basic security buffer should be a thick layer of unit tests we all love and write not even thinking exactly why, and not some lame code inspections no one have time or willingness for. By the way, there is no need to check the code after a guy with 20 yrs of industry experience, he stopped making mistakes a long time ago!”

Riiiiiiight...

According to Steve McConnell, author of a great book, “Code Complete”[1], doing formal code inspections can, on average, find twice as much bugs as just unit testing.

At first glimpse it might seem a bit contrary to common logic that might tell you that the person who knows most about given part of the code, and therefore is best prepared for its validation in terms of quantity of potential bugs is the author of the code himself, performing said validation using unit testing.

Unfortunately, reality is often different, and in practice it is difficult for a person to capture the mistakes he is the author of because he can not subjectively look at his own work, and tests, well, let's be honest, very often arise almost from compulsion, and test only happy paths, easiest to predict. And we all know, most of the time bugs are caused by an execution flow entering the state we could not predict while writing the code/tests, and even with some new, trendy and sometimes completely useless techniques like Test First or TDD, we can not avoid this.

In the article "What We Have Learned About Fighting Defects"[2] created on the University of Maryland, based on the research, authors confirm the thesis saying that code reviews help to catch around 60% of bugs, regardless of the domain of the problem, maturity of the organization or a lifecycle phase of an application on which reviews were conducted.
Another interesting observation was done by Victor Basili and Richard Selby in their publication “Comparing the Effectiveness of Software Testing Strategies”[3] - compared to functional testing, code reviews have been able to catch 80% more errors within an hour.

Why is then doing code reviews still not a common thing in lot of companies nowadays?

Is it because of lack of understanding the principles of code review methodology, or perhaps because of some false beliefs, rooted in the minds of programmers long time ago, for reasons not fully understood?

False beliefs, such as:

I have no time for that!

This is one of the most common reason for code reviews being done not often enough, and in a way that raises many doubts. Indeed, the repository is bombarded with new commits from many developers, constantly. That might indeed raise your concerns about hundreds, thousands or even millions of lines of code to be reviewed every day!

But you should check how it really is. I was working for quite a lot of different projects - mature ones with millions of legacy LoC’s and tens of developers, some greenfield ones, for big companies, small companies, good, bad, etc. And almost in each of these, the size of daily checked-in codebase was a reasonable amount to be reviewed. So next time you might want to say you don’t have time to review this “huge amount of code!”, open your terminal, do some git diff --stat or whatever VCS magic you want, and see if it’s really an issue.

I don’t want to do this, it’s boring and repetitive!

It is hard to argue with this approach, but it is worth trying. The reason for such thinking may be the misconception that code review is a tedious process of reading a line along the line of another programmer's code, to find typos, or obvious mistakes or potential 'holes'

However, this process can be received completely different - it is difficult to overestimate the educational value of such a process, both in terms of the craft itself: “Oh, that’s a nice trick!”, or the problem domain: “Oh, so this is how it works”. In addition, a more experienced colleague will help us capture our not-so-good habits, which we do not realize we have, and cause us to make an embarrassing mistakes from time to time.

Introducing the next phase of the code integration process will delay its production deployment!

Do not be fooled, the vast majority of changes are not significant enough so that delay in integration for a day or two has brought measurable losses. Sure, there are cases of urgent fixes, which save our ass, but usually they are one or two-liners, which could be checked by another person within a few seconds. And it's hard to imagine a more embarrassing situation than one where the fix of one thing spoils another, or, worse, it does not fix the original bug. And the stress caused by the unexpected fuck-up or the rush caused by the menacing look of a Business, does not make it easy for us to make flawless fixes. On the other hand, there are exceptions to every rule, and no one will harm us if we try to commit some crucial lines without asking for a review.

---

OK, let's assume that we have managed to eradicate any harmful beliefs and habits, and the whole team is ready to take on the burden of regular code reviews. So how do we start? How to avoid the chaos created when a dozen or so people rush into the unknown?

It will not be possible without establishing certain patterns of conduct, and the rules that should apply to each code review (both the applicant and the reviewer).
No worries, these are not some upside-down changes, but rather probably cosmetic patches that are good to use in our daily work anyway.

Let’s take a look at them:

Coherent code style

In my opinion this is a fundamental step to make the reviews easier and more optimal. An company-wide code format will avoid unnecessary misunderstandings about how to break and end lines, insert brackets and parentheses, or name variables or methods.

It may seem like a trivial problem, but in reality it can avoid unnecessary time wasting while reading the code. Just one empty line is enough for the brain, accustomed to single intervals, to pause for a moment asking the question, 'Is this the context change? Is this two-line interval a result of conscious action, error, or negligence? '
If the class is short and fit on one or two screens, it's ok, but reading a thousand or more lines monster, which has been deliberately shattered with unnecessary lines, can already give you a little headache.

It is also much easier

to read the text, which

is written quite succinctly,

so that we are able to see

more of it at a glance.

Right?

Establishing a consistent coding standard will also eliminate the majority of style-related comments: 'I would break the line by X, and would move Y higher, with 4 spaces indented'.

In conclusion, it is much easier to read code/text that is subject to some well known rules, because after some time any deviation from the norm our brain will be able to catch up almost immediately without engaging our consciousness, which should be concentrated at analysis of the logic, not syntax.

This is a trivial change to make, as each IDE provides the ability to import the appropriate code formatter (such as https://github.com/google/styleguide), and configure it to make
things happen automatically.

Emotions aside

Liking and antipathy are known in every environment where two or more people are dealing with each other. This can not be avoided, however, you can try to limit the impact of such social affinities on our work. Code reviews are not meant for bullying the author (not without the reason it is called a Code review, not a Coder review), or building your own ego, convinced that the other developers are the bunglers. There is no place for indulgence though, every, even the smallest mistake should be mentioned, even if it would involve adding 10 comments to 5 lines of code - and this should not, under any circumstances, cause any negative emotions (on both sides!).

I have often come across a question from a new team member who has not yet mastered the code review process:

"In my first change-list, I received 60 comments, would not it be badly received by anyone?"

No, because it means that the production will land about 60 ‘mistakes’ less than if the review did not take place. And the author got about 60 valuable, future-relevant comments that would make a good contribution to improving the quality of his code.

Less is more

We have previously determined that the number of changes made in the code is often small enough that looking at everything should not take a noticeable amount of time. But viewing 10 change-lists of 50 lines each is one thing, and checking one with 500 lines is different. In the first case, 10 reviews can be distributed between several reviewers, or even if they are done by one person, they can be spread over time without losing the context ('Where did I stop?').

It is therefore good to make sure that there are some loose limits on the number of changes in individual commits. Once that will make it easier for the reviewer to work, and two, it will force the programmer to split the changes, and thus avoid creating huge commits that change half the system at once. Of course, it is sometimes necessary to introduce a global change resulting from the necessary refactoring, which can not be divided into several stages, but this does not happen very often.

“Code reviews are not only about WTFs, but also “well dones””

Exactly, do not forget this important principle. Many people live in the belief that code reviews are just for finding errors in the code of their colleagues. This is a misconception of the purpose of this process, concentrating only on one of its aspects.

Why, if we see a piece of code that is, at least, of dubious quality, we are more inclined to pay attention to it than to praise engineering greatness in the form of a well-implemented algorithm that speeds up execution time by 70%?

Human nature, that’s why.

However, it is worth remembering that on the other side of the cable is a human being, a programmer, just like us, who will surely be happy if in the midst of criticisms (well, not exactly criticism), he reads something like:

"good job!"

Process integration

Incredibly important point, without which all the talk about code reviews loses the slightest meaning. Code review should not be a process that is somewhere on the side of the main code manufacturing process.

Like unit tests we do not write in 'free time' or when there is nothing to do, so code reviews should not be done selectively and only in specific cases. Review should be a necessary step before integrating changes with the main branch of development.

A situation where code is reviewed only after such integration (and thus after deployment!) raises many problems. If a bug or potential threat is detected during the review, the person who made the change will certainly be thrown into a whirl of fixes to eliminate the bug.

However, it may turn out that the patch has already been made by someone else, or that the code has changed so much that the cost of a 'quick' patch has risen to the level of a design challenge, or that a patch that has been introduced will cause further errors, which no one had the opportunity to catch, because, after all “review was already done”.

Most often, however, we encounter a situation where the developer simply will not create/forget the review, and the code will casually fly to the main branch.

In the age of advanced tools supporting development, the implementation of such flow should not be the problem at all.

---

I have not been a great fan of reviews ever since, I treated it as another thing to do. It was not until joining the team that the code reviews were almost a ritual without which the manufacturing process could not take place, after initially being discouraged by overburdened reviewers, I understood how important this aspect of software development was and how much it could give us. These minutes or even hours devoted to reviewing the code of colleagues can really save 10 times more in the future, which we would devote to fixing mistakes if our colleague did not pay attention at the right time that:

“this code won’t work, mate, please fix”.

---

[0] image is a property of Manu Cornet
[1] https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670
[2] http://www.cs.umd.edu/~mvz/pub/eworkshop02.pdf
[3] https://www.cs.umd.edu/~basili/publications/journals/J34.pdf