Code Reviews (FTW!)

Code reviews are so unbelievably important for a team, I can’t believe how many teams don’t do them. And it isn’t just the development team, its any team that writes code. Dev, QA, Database, Ops, everyone! If your customer service team is writing macros in Excel, they should be code reviewed!

Let me just start by acknowledging that even a thorough code review won’t catch every issue. Code reviews are pretty good at catching certain types of issues. Silly errors like using an assignment operator instead of an equality operator, or a strict comparator instead of non-strict. These are mistakes that are easy to make, and can often have very subtle consequences that are hard for your tests to catch. Fortunately, they also tend to be the types of errors that stand out to code reviewers (probably because they are simple and everyone’s looking to score points on the review).

Code reviews are also pretty good at catching errors in business logic, since the combined domain knowledge of the reviewers is probably greater than that of any one developer.

On the other hand, there are certain types of error that code reviews are really bad at catching. Errors in complex or arcane logic are not likely to be detected, especially in a live code review. Incorrect assumptions or incorrect use of third party dependencies are not likely to be caught, either.

The good news is that your unit tests and integration tests should be pretty decent at catching these things.

A lot of people argue that if you’re doing sufficient testing, you don’t need to do code reviews. Again, there are certain types of errors that may have subtle effects that are hard to test. But the real problem with this argument is that it misses all the other benefits of doing a code review, the side effects.

First of all, code reviews raise the bus factor. This is the minimum number of team members who would need to get on the proverbial bus in order to incapacitate your team. A bus factor of one is bad. It means that if just one team member is out for an extended illness or vacation, or decides to leave the team, you’re team is sunk. You’re going to be scrambling for the next 4N weeks to try to ramp up on the work they were doing. Why? Because they were the only ones who knew what they were doing. They’d been working alone on a code base for the last N weeks and nobody else had ever seen their code.

So before your team members decide to head straight from their silos to a permanent vacation, raise the bus factor. Have code reviews. Get more eyes on that code so other people start getting an idea of what’s going on, and maybe you can get that 4N ramp up down a bit.

So the first side effect of code reviews is about sharing product knowledge. The second side effect is sharing patterns. Most of your devs probably have a different background, different education, and different exposure. There are a lot of “standard” design and coding patterns out there, its unlikely that all of your devs know all the same ones. Doing a code review is a way to get a free education in patterns the author uses but you’ve never seen before. This is great for the team at large, because the more standard tools you have to draw from, the more effectively and the more safely you can write high quality code.

In addition to “standard” patterns, there are likely to be commonly used patterns that are particular to your team as well. By exposing more of your team to these patterns, you increase homogeneity across your code base, which can make it much easier to move between different projects, or pick up the work of others when that proverbial bus comes along.

The third side effect is along the same lines, except in this case its about identifying antipatterns. Antipatterns are a thing specifically because they are things people do. If they were bad ideas that nobody every did, there probably wouldn’t be a name for it. Generally people do them because they’ve unaware of the negative consequences (though of course sometimes we have a good reasons to use them, and sometimes we’re just lazy). Code reviews are a great way to share knowledge about antipatterns so devs can stop using them.

So code reviews aren’t just about code correctness, they aren’t even just about code quality for the code being reviewed. They’re about global code quality, for the entire team. And if that’s not worth a few hours a week, I don’t know what is.

Code Reviews (FTW!)

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s