The Shape of the System

Reading Is Harder Than Writing

The job of a code review was never to find your bugs. It is to drag what one person understood into the open, before it sets like concrete.

reviewing

The change was small and it sailed through. Tests green, two approvals on it already, the author quietly pleased with herself. Then a third engineer read it properly, maybe for the first time anyone had, and she stopped on one line and asked a question that nobody else had bothered to ask, which was: what happens here when the request comes in with no account attached to it? The author started to answer and then realised he didn't have one. The code wasn't wrong. It just took something for granted that happened to be true on his machine, on his particular afternoon, with the whole shape of the thing still fresh in his head, and was true pretty much nowhere else. No test had caught it. The person who wrote the tests and the person who wrote the code were the same person, and he'd imagined the same neat inputs both times. It took someone who was a stranger to the code to spot the hole, and the only reason she could is that she walked in with nothing but fresh eyes and none of his assumptions.

We tell ourselves review is there to catch bugs. That's the comforting version. A second pair of eyes, a net under the trapeze, the place where defects get stopped before a customer ever sees them. Mostly that isn't what review does though. Google went and studied its own review culture across nine million changes, and the thing that stood out was how far the whole practice had drifted from hunting defects. In their words it had turned into less a way of finding faults and more a way of solving problems together. Microsoft researchers did something more hands-on. They sat down and sorted a heap of review comments by hand, and the same fact came out as a number: fewer than a sixth of them were about defects at all. Everything else was other stuff. Is this readable, is it named well, will the next person follow it, does this even belong here in the first place. So the bulk of review is really about whether the change makes sense to someone who wasn't in the room when it got written. Calling it "code review" is nearly a lie. It ought to be called code reading, because the reading is the whole thing.

And the reading is the harder half by a long way. When you write code you build a model in your head and then you write it down, so the model comes first and the words are just its shadow. Reading, you've got the shadow and nothing else, and you have to put the model back together from bits, out of order, missing the thousand tiny decisions that were obvious to whoever wrote it and never got written down anywhere. That is hard work, and it's where a reviewer's attention actually goes. Most of the effort in a review isn't a hunt for flaws. It's comprehension, plain and simple, just trying to keep enough of the change in your head to follow what the thing does. Push a reviewer past what they can hold and they don't suddenly start turning up subtle bugs. They start nodding along. They check the indentation, they check the variable names, the stuff you can look at without understanding any of it, and then they approve, because the surface is all that's left to them once the model won't fit any more.

None of this is new. The first person to turn code review into a measured, disciplined thing was an engineer at IBM called Michael Fagan, back in the nineteen-seventies, and what he found wasn't that inspection caught every bug. It was that reading the code slowly, together, before it shipped, pulled the defects forward in time. Out of the expensive future and into the cheap present. The value sat in the discipline of the reading, not in how clever the readers were. And the defects that reading is best at catching are a specific sort. Not the violated spec, a test will find that. The invisible assumption. The dependency tucked away inside a constructor. Some bit of state that got changed quietly three frames back. A global that makes a function behave one way or another depending on what happened to run before it. These aren't bugs in the sense of breaking some rule. They're gaps in understanding that have been baked into the shape of the code, and the reason the author can't see them is that the author is carrying the missing context around in his head. A cold reader isn't. A cold reader is the only one who'll ever feel the gap, because to her the gap is all there is.

There's a second thing review does, slower and quieter, and it might be the most valuable part of the lot. When you read someone else's code, the way they did a retry, where they re-checked a permission, the shape of some operation they made safe to run twice, all of that goes into your head too. A team that reads each other's code slowly drifts towards one shared idea of how things get done around here. Stop reviewing, or review without actually reading, and the team comes apart almost at once, everyone building in their own dialect, and before long the codebase is a string of neighbourhoods that don't quite recognise each other anymore. The knowledge passing between people isn't some nice side effect. Over a long enough stretch of time it's the main effect, the thing that keeps a system understandable by more than one person at once.

So here's what a review is for, said plainly. Not to find your bugs, because tests and time and your users will find most of those anyway. It's the last cheap moment you get to move correctness out of one person's head and into the shape of the code, before that code hardens into something a tired stranger has to maintain at three in the morning, having never met you. A good review asks whether the next reader could follow this without the context you happen to be holding. Whether the wrong thing could even be written here at all, or whether the structure has already ruled it out. Where you're being asked to trust that the author remembered something, instead of being shown that the code simply can't forget it. Catch one of those and the review has done its real job, which was never about catching a bug. It was about closing the distance between what one person knew and what the code makes plain to everyone who turns up later. The author wrote it once, fluently, in a language only he really spoke. The reviewer is the first person to translate it, and the translation is the whole point.

In the manifesto, this is tenets (I) and (XIII).

Sources

One of a series of field notes on building software for the way minds actually work: tired, distractible, ordinary, and now partly machine. They all lead back to the manifesto behind them, The Shape of the System.