Code Review Musings

build it

Pro Tip: While waiting for PR approval, you can write haiku

Introduction

I've been collecting some thoughts about code reviews over the years. I've worked at (very) large engineering organizations and (very) small ones. I've heard misguided arguments and dogmatic edicts. I've read insightful, new approaches and I've evangelized them to blank stares.

Some folks do push back. They show me their "Code Review Guidelines" from their company's "Software Standards" that endorse gated, pre-integration code reviews. I ask them "How does that align with Continuous Integration (CI)?" or "How many bugs have you caught this way?" or "Does this sound Agile to you?" Often, the blank stares resume.

This article is not a scientific exploration nor a formal proof. These are just some of my thoughts that I hope will provoke a few of your own. Of note, I can see a couple of threads woven throughout, and perhaps I'll pull on one or two in the future. For now, here are my observations.

Software Evolves

Distasteful engineering processes often involve a "One-and-Done" way of thinking:

Manager: "Hey, what are you doing in the Contacts service? We finished that last sprint."
Engineer: "Well, I just realized, while working on the User service, that we can use this new library to simplify things in both services."
Manager: "That's not in the sprint; there's no ticket for that. Besides, we're done with Contacts."
Engineer: "..."

Software evolves over time, just as our knowledge does. It grows as we gain new context. "One of the most pervasive perspectives in software is the notion that it's something we build and complete" (Fowler)¹. A new technique learned today can improve the service you wrote last month. Further, it never ends. Just as you never stop learning (you are still learning, yes?), your code can always become better - and by "better" I mean "easier to read", "easier to understand", "easier to modify", etc.

Code Review Goals (or "I don't think this means what you think it means")

Many companies profess that they use pre-integration code reviews so they can catch bugs. I even had an engineering lead once tell me "I can practically compile the code in my head[!]"². I think catching bugs is laudable. However, a pre-integration code review is the wrong tool for the job.

In my codebase, if I'm interested in ensuring bugs are minimized, I'm going to focus on tests, lots of good tests. A set of highly-curated tests lets me know whether the app does today what it claimed to do yesterday. No other process comes close. And few other processes are worse at bug detection than a human reading over a few new sections of a code change and trying to predict their impact at runtime. And I hope I don't need to explain why a computer is a better tool for compiling code than your brain.

There is so much more to say here: giving engineers space to fail, encouraging smaller changes that integrate quickly, encouraging refactors, getting feedback from the only real source of truth (your master branch - deployed!), etc. Please see the references section (below) for more on all this.

Anecdotally, the many (many) pre-integration code reviews in which I've participated rarely caught bugs. Nearly all the comments were about style, readability, and modularity. All of these are good things, but they are not worth preventing someone from integrating continuously. All these issues can be addressed afterward, in a subsequent change (remember, we're not "done" yet - keep refining).

My Preferred Approach

I'd like to quickly touch on my preferred approach to handling code reviews. I do them post-CI (post-merge) and I do them continuously. I did not invent this process and give much credit to Martin Fowler who wrote extensively on the subject. He calls this "Refinement Code Review" while I often call it "Continuous Code Review" but the idea is the same.

And I'm not fundamentally anti-pre-integration review, I'm pro-continuous-integration. You can have a pre-integration review and continuous integration if you do pair programming. The review happens as the code is written and doesn't slow down CI at all.

One interesting point of post-merge Continuous/Refinement reviews is that they happen in the code that gets read the most often. And, since refining for clarity improves the readability of code, this is exactly the part of the code that needs it. Conversely, code that is not going to be viewed for another six months doesn't need an up-front, gated review. I won’t give all the details here - read Fowler's blog post.

Trust Relationships

Here's the one thread that I'm going to tug on today: trust. So much of the discussion around code ownership involves who you trust to make changes and who you don't. Let's look at two general categorizations:

• High-Trust: the team that owns the code. Ownership implies running it, monitoring it, getting up at 4 a.m. for an issue, etc. This is almost always within a private organization.

• Low-Trust: the other teams in the company (if private); almost everyone else in the world (if open-source)

You cannot mix and match high-trust teams with low-trust flows without courting disaster. That is, if another team in your company has "away work" (more on this, below) that they are contributing to your code base, you can treat it like OSS. Further, you should not be under any pressure to accept their changes.

For example, let’s say someone in Product wants a new parameter stored in a system owned by Team Alpha so Team Bravo can use it. Team Bravo makes a PR in Team Alpha’s code base because Team Alpha is consumed with higher-priority (according to their product owner) changes. Team Bravo must wait until Team Alpha has time to invest in properly reviewing their pull request. If you bring in a higher authority (VP/CPO) to force Team Alpha to accept the change because of "deadlines" or "commitments" that Team Alpha did not make, you have smashed trust all around. I've witnessed this at multiple companies and the all-too-predictable demoralizing effect is tragic.

The OSS Fallacy

You are probably not building Open-Source Software (OSS). Don't run your team like an OSS project.

So many companies use pre-integration (gated) code reviews like they would if they managed an open-source project on GitHub. OSS is inherently low-trust; you need to protect your master branch from contributors you don't know. Pre-integration reviews make complete sense here.

However, your full-time engineering team at your company is a high-trust situation. There should be constant, high-bandwidth conversations about their code. Encourage discussion, support continuous integration, and treat the team as if you trust them.

The "Away Work" Fallacy

I have noticed that the time to thoroughly review and refine an away-work pull request is roughly equivalent to the time it would take for the team that owns the code to do it themselves. This is often the result of two (or more) Product Owners clashing on priorities and one attempting a flanking maneuver on the other to ram a change through. If allowed to happen, the Engineering team pays for the Product Team's conflict. Recognize this situation and stop it early. I think this conflict within Product is inherent in all "away work" and the whole practice deserves further scrutiny.

Summary

Again, the goal should be continuous integration. A pull request stops integration in its tracks. Take a minute to ask yourself why you are doing the things you are doing. Be sure your answers align with reality. If you do try out Continuous/Refinement reviews, I wish you well. I think you'll enjoy the newfound bump in productivity. Have fun.

References

The Agile Manifesto
Martin Fowler Tweet: refinement CR and article
Martin Fowler: Branching Patterns
Rouan Wilsenach (on Martin Fowler’s blog): Ship / Show / Ask
Martin Fowler: Continuous Integration
Bockeler & Siessegger (on Martin Fowler’s blog): On Pair Programming

1. Refinement Code Review by Martin Fowler

2. You, my dear friend, shall remain anonymous