@reiver

Code Review

A code review is where one person reviews the source code for software written by someone else.

Code review” is short for “source code review”.

Code reviews can be a powerful institution to catch software defects, if what is checked for by the reviewer is restricted to just software defects.

However, there are a number of ways that the practice of code reviews can go wrong, and create a lot of problems for individuals, for teams, for departments, for companies, and for communities.

And in those cases code reviews usually end up doing more harm than good.

For example, it seems common that the institution of code reviews is often expanded to include other things (beyond just software defects); and this seems to lead to a situation where the code reviews start doing more harm than good.

But for the situations where code reviews can be restricted to just software defects, they can be a useful institution.

Code Review Checklist

These are the things I would recommend you check for on a code review.

I strongly recommend you do NOT add anything to this checklist.

It is dangerous to check for anything more than this, for reasons I'll explain later.

Here are the things I would recommend you check for when performing a code review:…

  • bugs,
  • missing error handling,
  • resource leaks,
  • security issues,
  • computation complexity issues,
  • user experience (UX) issues.

That's it!

Code Reviews Considered Harmful

Code reviews can be a powerful institution to catch software defects, but only if what is checked for by the reviewer is restricted to just software defects.

What I've noticed is that the other (non-software defect) things (i.e., things beyond the things on the list I provided) often become part of the code review.

In fact, while restricting code reviews to just software defects may sound simple, my experience suggests it is very very difficult to end up in that situation.

Whether intentional or not, the effect of those other (non-software defect) things being added to code reviews seems to be:

  • feuding among those writing the source code for the software,
  • which includes battles for dominance, and
  • back-and-forth attempts to create pecking orders.

The result of someone being on the receiving end of this kind of behavior seems to (understandably) be that:…

  • these individuals become angry,
  • and try to struggle, and fight to change things as an individual,
  • and then, if that fails, try to get others to join them to struggle, and fight to change things as a group,
  • and then, if that fails, then eventually later give up and become unhappy.

For some individuals, it stops there. These people hate their jobs, but still show up. But do only the very minimum to get by. Sometimes less than the very minimum. Individual who stop at this stage often do so because they have no other options in life. Either because of low experience, or low talent.

However, some individuals who get to this ‘give up and become unhappy’ phase (after being in the ‘get angry’ phase) do something different:…

  • some of these individuals quit and go find another job.

There are (at least) two problems for a company when this happens.

№1: individuals who become unhappy due to others dominating them tend to behave in certain ways (that the various behavioral sciences go into much detail over). This often causes internal politics, and internal fighting to increase, and intensify. This is not a good situation for a company to be in. As they can induce other people to quit too. And they can induce the ones who don't quite to become less productive.

№2: the individuals who are most likely to quit after experiencing the ‘get angry’ phase, and then later experiencing the ‘give up and become unhappy’ phase, tend to be the most highly experienced, and the most highly talented. Why‽… because they are likely going have the most career options. They are probably getting regularly solicitations from recruiters, and others trying to hire them. And they were probably ignoring them while they were content working at the company. But once they become unhappy, they are more likely to become open to these recruitment solicitations. For a company, these are the individual who you should most want to stay.

Having a situation where the most skilled, and most talented individuals writing the source code for the software are quiting because of this is a problem. It is a big problem.

Code Reviews Considered Harmful: An Example

Examples often help make things clearer. So here is an example of where it was me who created the problem via a code review.

From 2004 to 2007 (inclusive) I was involved with a startup. In the beginning I was the only person in the company doing the programming. I was the only person in the company doing the software engineering.

Actually, back then I did all the technical things in the company. So: programming, software engineering, software development. But also: system administration (sysadmin) (i.e., what nowadays gets called: operations (ops), DevOps, and SRE), DBA, technical project management, etc etc etc.

Later on in 2004 we hired another two software engineers.

I felt a personal attachment to that code base, and technology in that startup. I created. I spent a lot time creating from scratch.

At one point, one of the two new software engineers wrote some code, and I took a look at it. I.e., I did a code review.

What I noticed in his code is that he wrote his if-statements like this:


if (condition)
{

}
			

That bothered me. It actually bothered me a lot. It bothered me because I write my if-statements like this:


if (condition) {

}
			

I.e., I put the left curly bracket on the same line as the ‘if’, where he puts the left curly bracket on its own line after the ‘if’.

I wanted him to change his if-statements to match my style.

I spend some time effectively demanding that he change it. Demanding that he match my style.

We argued about this for a while.

All it did was make him angry. But here is the thing… he was perfectly justified in being angry.

Regardless of whether that was how I thought about it at the time, or not. I was effectively just trying to force my will on him.

And I was trying to force my will on him because of a style issue.

Code reviews where the reviews checks for things beyond just software defects, such a style issues, such as this style issue, institutionalize what I did those many many years ago.

Here is the thing, this whole style (actually any style issue) does not matter! It doesn't matter! It really doesn't matter!

This should never be part of a code review.

These type of things (these fights for dominance) will create various social issues between individuals, within teams, within departments, within companies, and within communities.

If you are going to work to others, you need to find ways to get along. And it is probably better that those are win-win positive-sum ways of getting along.

-- Mirza Charles Iliya Krempeaux
See Other Topics