A fundamental challenge when introducing reviews is that reviewing code is hard. This post summarizes our practices to nevertheless make life for a reviewer as easy as possible.

Do we need better code, or a smarter reviewer?

When I commit a piece of code, I know it intimately: I have thought of alternative implementations, tried some, and implemented the one that fits best; I have executed the code on different inputs and run a debug session or two; I have cleaned it up and prepared it for review. At this point, I have a deep understanding of the involved concepts, entities and interfaces:

When I review a piece of code, I do not know it well: I have not pondered alternative implementations; I have not executed or debugged it. Instead, I am faced with unknown concepts, entities and interfaces. My understanding is a lot more shallow:

In consequence, it often happens that I cannot completely understand the code I review. Even if I make an effort, it remains unclear to me why some aspects are implemented the way they are: why is this exception ignored—can it never be thrown? Why is this value here never null? Why is that sorting algorithm more suitable than the one we usually use? What on earth do variables p, q and r from that algorithm paper stand for, again? In other words, do we need better code, or a smarter reviewer?

If the reviewer does not understand the code, the code must be improved.

It is a problem of the code, not of the reviewer. Why? Because a couple of months later, even as an author, I often cannot answer these same questions, since I have forgotten the discarded alternatives and debug sessions. All information needed to understand the code (except for programming language knowledge) must be in the code or in its documentation.

As a reviewer, I need to point out the parts I cannot understand to help the author address these problems. It is not my responsibility to understand the code perfectly—some code is so messed up, it’s just impossible to understand. It is my responsibility, however, to point out all unclear points.

Who implements a fix?

In some cases, it seems that describing a problem to the author takes about as long as quickly implementing the fix during the review session. So who should implement a fix? In our team, we have this default rule:

The fix is implemented by the author.

There are a number of good reasons for this:

1. The review goal is to increase code quality as much as possible. However, if I need to fix the problems that I discover, I might tend to ignore some, decreasing my review effort.
2. Knowledge transfer is better if the author fixes the problems. If I would fix the problem myself, the author does not learn about the problem and probably does the same thing again.
3. The author has a better understanding of the code. In many cases, he knows a more elegant solution.
4. Even if it seems that implementing the solution takes about as long as describing it, it often takes longer and affects more code in reality.
5. Changing code during reviews blurs the boundaries between author and reviewer. What about problems that I introduced during my fixes?

Exceptions

This rules covers default cases that work 95% of the time. Exceptions are:

1. The fix is trivial. If I find, for example, a misspelled variable name or a typo in a comment, I fix it directly. It is quick, the author would learn nothing from fixing the typo himself, and the risk of breaking the code is negligible.

2. It takes a lot longer to describe the problem than to fix it. In that case, we reverse roles: I take over as an author and the author reviews my change. This way, the boundaries between author and reviewer are maintained. If my solution has problems I overlooked, however, it’s now my job to fix them.