Code reviews

It has been a long time since my last post, but I finally had a chance to sit down and write. I have been thinking about this topic for a while and I decided that the best was to put my thoughts in a blog post, so here it goes.

I have been doing code reviews for as long as I have been writing code, and that is a long time since I started when I was about 10.

The first times I did code review it was not a formal code review in the normal sense. At that time I used to get a magazine once a month that will contain the listing of a program, usually a game, that you had to type by hand and at the end you would hopefully get a new game to play. Unfortunately most of the time the code did not work, so I had to start looking at the code to try to find what was wrong with it.

Fast forward a few years and I started reviewing code from colleagues or from other people collaborating on open source projects.

At first my code reviews were about “finding flaws” in the code to try to shut down the code. I think this is a very classic starting point for code reviews. One starts by finding flaws and exposing them.

Over time I have improved, at least I hope so, my reviewing skills and develop my own way of performing code reviews. I have also realized that just focusing on the code at hand is a very poor way of doing a code review, so at some point I started adding dimensions to my code reviews and now I review code in four different dimensions, two of them intrinsic and two of them extrinsic to the code:

• Delivery
• Structure
• Functionality
• Critique

I will explain each one of them, but it is worth performing a review in that order. Do not jump directly into code critique, take the time reviewing the other dimensions before doing code critique.

Before jumping into how to perform a code review, it is worth to separate code reviews into internal and external. Internal code reviews are reviews of code over which you have a certain degree of control. For example code that is written by a colleague in your team or from a collaborator in an open source project. External code reviews are reviews of code over which you do not have any or very little control, for example colleagues from a different part of your organization or sudden contributions from somebody to an open source project. There is not much difference in the way the review is performed, only a small details that I will highlight when needed.

Delivery

This is probably the most overlooked side when reviewing code, but in order to review code it has to be delivered to you somehow.

This usually happens in one of three ways: a pull request (or equivalent in your VC), a patch file (or a collection of them), or a package (such a zip file or a tarball).

Start by looking at how the code is delivered and take your time. Usually when people ignore this, they get upset really easy if things do not go the way they expected, so instead take the time to find out how it should really go.

As a rule of thumb I always start by looking at the contents of the package or by looking at the files included in the PR or patches. Is there a file with instructions (a README, BUILD or similar)? If so, take the time to read it and find out what you should do. This file should also document the requirements to build the software, for example which version of the compiler and potentially what other software you should have in your build environment.

If there are no clear instructions, that is usually a bad sign. Not only because there are no clear instructions, so you do not know if the build fails because you did something wrong or because there is a problem with the code, but because not including instructions usually leads to base your work on assumptions that might not be true.

Here there is a difference between an internal and external code review. An internal code review should follow the standards and conventions, so instructions are less relevant because the build process is already documented. But if a new component or requirement is added, then the corresponding build process should be documented.

Structure

This dimension refers to how the code is structured. This means, does it follow the conventions of the language? For example, C projects usually have a folder called include where all header files are stored.

In Java the code is divided in packages, with the idea that code that has similar functionality should be closer. There are many ways to divide the code into packages, so I will not go there but there should be consistency in the way it is done. For example, if there is a package called exceptions, are all the exceptions there or are they spread all over the code too?

Another aspect of the structure is the logical division of the code. Are things grouped by their functionality or are they just grouped randomly?

Functionality

Usually people think about this dimension, but only after criticizing the code. This has the consequence of overlooking aspects that might not be obvious.

The first thing to look for is a description of what is the functionality that is accomplished by this code. This ties directly with the delivery of the code, is there a link to a task in a task manager? Is there a design document? Without those, it is very difficult to judge the functionality of the code.

When looking at the functionality it is a good idea to look at the code as a black box. Look at the unit tests. No unit tests? That is a red flag, no unit tests means that there is no easy way to check the internals.

Critique

This is the fourth dimension and should only be performed after you are finished with the other three. Most people jump straight into code critique and miss a lot of information.

Criticizing code is not easy and it could be argued that is highly subjective. Therefore it is necessary to have seen at the other aspects of the code before doing it.

When reviewing the code, look first for consistency. Does the new code look like code somewhere else in the project? If it does not, then ask why. Is there a valid reason for introducing a new style?

Does the code use the right methods, classes and functions? For example, if searching for text, does it use regular expressions or does it use a brute force approach?

My advice when criticizing code is to focus on the big picture. Does this code fit nicely into the project or does it feel like it is being forced into the project? This is more subtle than what it looks like, if the project is making use advanced constructs of the language, for example lambda functions, but this code is not then it is worth asking why not.

Another aspect worth to keep in mind when criticizing code is to consider the logical flow of the new code. Again this is trickier than what it seems since you have to be able to look at the code that will call this code and follow the logic through the whole call path. Sometime ago, in a previous team, somebody suggested a change that would introduce state into a component that did not have state before. The problem was that the state would only be introduced on certain occasions, which would lead to a situation in which some threads would have state and some other would not. After a discussion, it was decided to drop the change.

Final words

Before I finish, I would like to say that code review is a great opportunity to learn new things. Approach code reviews with an open mind, not with the idea of shutting down code.

The objective of a code review is to make the code better than what it was. Sometimes the code is already at the level it requires and there is not much to say, but usually there is some detail here or there.

Sometimes you might find yourself in a position where you do not understand the code that you are reviewing. Do not be afraid to ask about what is happening in the code, if you do not understand the code now, chances are you will not understand it later when it is running in production.

And as a last point, a code review is the place to ask questions. Even if there is a task that says that something needs to be done, if you feel that you need to ask about the task or about the implications of the change, do not be afraid and ask politely. The person that wrote the code, might not understand it either, so by asking the question you might lead to a much better and improved code or to drop a piece of code that might lead to problems later.