{
    Adam Ard
}

Code Reviews are Broken — Here is How to Fix Them

In an environment already filled with painful interruptions (open office layouts, Slack, email, meetings, managers), code reviews are yet another task that divides an engineer’s attention. A good code review requires a great deal of energy; understanding someone else’s code is one of the more taxing things you can do as an engineer. Is it really worth the extra effort, time and distraction? Not the way most code reviews are done today.

Occasionally management will make a checklist of items that constitute a good code review. A typical checklist may include: running the code and confirming that it works, confirming that all tests pass, checking that the coding standard is adhered to, checking that all duplication is removed, making sure functions/classes/files are not too large, checking for any code that is commented out. These lists go on and on. Soon enough, the effort of doing a code review has doubled or tripled. But the quality of the average code review still doesn’t seem to increase. That’s because all these checklists are missing the most important consideration: Is the code reviewer sufficiently invested in the code being reviewed? If not, then don’t bother.

How can you assure that a reviewer is sufficiently invested in a piece of code? The answer is code ownership. The person that reviews code needs to own the code repo it is meant to modify. Only then will they care enough about the quality of the incoming code changes. If you are using something like git, the code owner would be the person that has write access to the target repo — the only person with write access. If more than one person has write permissions to the repo, it is not ownership but collectivism, and code collectivism results in the same old apathetic reviews.

When engineers collectively own code, their review often goes like this:

You are in the middle of an assignment on which you are concentrating deeply. Someone taps you on the shoulder even though you are obviously busy.

“Can you do a quick review? It shouldn’t be too bad. I just need to get this in and everyone else is at lunch”

You agree and open their merge request. You are hoping that they just renamed a few variables or something, but unfortunately, it’s more complicated than that. You can feel your brain frantically trying to hold the problem you were just trying to solve in the back of your mind. You look around and see that several files were changed and it is not immediately clear what is going on. To make things worse, this is a code base that you don’t spend much time in. Resigned, you can feel your brain letting go of your previous task— like a computer program that just corrupted its call stack. All the context you were holding is gone. Grumpily, you go through the motions of the review, knowing that in spite of your efforts you still don’t fully understand all the implications of the changes — that would take days. After a sufficient amount of time and effort, you approve the change.

Contrast that with how a review proceeds when strong ownership is in place:

You are in the middle of an assignment on which you are concentrating deeply. Someone taps you on the shoulder even though you are obviously busy.

“I just submitted a pull-request for your repo.”

“Oh nice. Just send me an email with the words pull-request in the title and I’ll get to it. There are more instructions on the readme for the repo”

“Ok cool.”

... 4 hours later ...

You finished the item you were working on and are now looking at pull requests in your inbox. You see the one that your co-worker referenced early that day. It adds support for importing a new file type— Awesome! You love free work. It is a little more complicated then you expected. It looks like they didn’t understand how to extend the importer class to add a new file type. They ended up writing a bunch of unneeded code. You email them back and tell them how to simplify it.

… the next morning ...

You check your email quickly before starting on other stuff to see if your coworker got the file import thing figured out. It looks like they did. You notice a few other minor consistency things, but instead of bouncing the pull-request again you just fix them yourself. In 30 minutes it is all merged. You are amazed at how fast your library is coming along with all these helpful contributions.

It is easy to see the difference. First, because you control the process by which pull-requests are accepted (email in this example) you avoid feeling like you have to drop everything to help your coworker. Further, you know who is depending on changes in your repo, and therefore have a better understanding of how urgent a given change really is. You aren’t left wondering if people are blocked on the review in question.

Second, you are deeply knowledgeable about the code being modified. It is therefore easier to spot any problems. In this example, you immediately notice that the code submitter is extending a class improperly. These are hard things to see when you don’t know the code. Even though the code being submitted may function, it may not be very consistent with the architecture.

Third, you are much more invested in keeping the code clean — because it is yours. If it is cleaner you can take pride in your accomplishment as well as reap the benefits of code that is easier to maintain. If you do a poor job with maintaining your code you have no one to blame but yourself.

Lastly, as opposed to feeling like a code review is an unwanted interruption, you are sincerely excited to receive pull-requests, because it means people are using your code, and it is getting better at a much faster rate. Your attitude and motivation is completely different

But wait. If everyone owns their own code, doesn’t that increase the bureaucracy? Doesn’t that just make it harder to get things done? Not really. Just because there is only one person with write permission to a repo doesn’t mean that others don’t have access to the code. Everyone at the company should have read access to all the code. As a result, anyone can view, comment on, or submit changes. But only the owner can merge it. If this person goes on vacation, no one is locked out of the code. Git is completely distributed. You can clone any repo and even modify it locally if needed. When the owner returns from vacation, simply submit any important changes as a pull-request and they can be incorporated into the main line again — with the important input of the author who holds the overall code vision and design. As the one that is totally invested, he will make sure that it all works and lives up to the standard that he is enforcing. This is the most useful incarnation of a code review — no checklists, just deep commitment and responsibility.

The final question that you are all inevitably wondering is: What if you are adding code to a repo that you own? Who reviews it? This may seem strange, but, the answer is: no one needs to review it. I think that you should be able to iterate as fast as you feel comfortable with in your own domain. Perhaps, you may want to ask people their opinions of your design decisions or get specific advice about other matters, but I don’t believe anything should be formally required. All the right motivations are in place to prevent you from doing stupid things and no one is going to know your code base as well as you do, so there is not much payoff.

In summary, for reviews to really be worth the effort (which is considerable when you take into account all the commits that engineers are making everyday and all the people that must be interrupted to review those commits), the code must be owner driven. This is the only way to be assured that the reviewer is invested enough to do a quality job.

P.S. There are a whole host of other benefits to code ownership that I will be writing about soon. Better code reviews are just the tip of the iceberg!