You learn more about an engineer from a refactor than from a feature. A feature has a spec to hide behind. A refactor has only judgement: what the author chose to touch, what they treated as fixed, and which questions they asked before deciding a line deserved to live.
That observation shapes how we work. The team works real issues on EkoLite, our small real time backend, and when a refactoring story comes back as a pull request we read it twice. Once for the code, and once for the reasoning the code implies. The gaps we find in the second reading are the curriculum. Not a curriculum we planned in advance, but the one the work itself just wrote.
Here is one of those readings.
What arrived
The story asked for a cleanup pass over two small classes: the shutdown policy we wrote about in The five second goodbye, and the nullable ProcessWrapper it leans on. The PR that came back was careful work. A duplicated guard extracted into a helper. A long signal handler moved out of arm() into a named method, byte for byte. A stub timer bug genuinely found and fixed: the fake clock fired timers in insertion order where the real setTimeout fires them by deadline, and the fix came pinned with a new test, with no existing test modified. Every test green, the description honest about what changed and why.
Nothing wrong in the ship it sense. Which is exactly when the interesting review starts, because a refactor that works is finally quiet enough to show you how its author thinks.
Reading the reasoning
Three of the changes, read together, told one story.
The extracted guard took a parameter. The old code had two identical checks, each throwing its own message: advanceTime only available on null instance, simulateSignal only available on null instance. The new helper reproduced each string exactly, so it needed the caller to pass its own method name, and the author typed that parameter as a strict union of the two names. You can see the reasoning: the messages are existing behaviour, existing behaviour is a contract, contracts must be preserved precisely, and strictness is free. What nobody asked is whether anything depends on those strings. Nothing does. There is not a single toThrow in the test file. The author preserved a constraint no one had set, and paid for it with a parameter where a caller can now pass the wrong name and ship an error that blames the wrong method.
The timer fix came with a cleanup line. After firing due timers, the new code compacted the internal array so fired and cancelled timers would not accumulate. Preventing growth is a good production instinct. But this array lives inside a test stub that exists for the length of a test and holds at most four timers, and the field it compacts is private, read by nothing except the loop that already skips dead entries. No test can fail because of that line. No test could ever fail because of that line. And the shape of it was revealing in its own right: the field was declared readonly, so instead of reassigning a filtered array, the code splices the array with a spread of a filter of itself, a small contortion whose whole purpose is to mutate around a declaration the author did not feel allowed to question.
That was the pattern. Every choice optimised for visible tidiness: deduplicate, tighten types, clean up after yourself, match the interface exactly. All respectable instincts, and none of them wrong on their own. What was missing was one question, asked of any single line before keeping it or adding it: what can observe this? Which test is this line answerable to? Two of the four threads we opened on that PR were that one question wearing different clothes. The other two were its mirror image: a behaviour a test should have watched, and didn't.
Teaching with a failing test
So how do you teach that? Not with a comment that says remove this. A verdict teaches the fix and nothing else. The rule we hold ourselves to as reviewers is the same rule we hold the code to: every claim gets backed by a test.
If the claim is about shape, the proof is the existing suite staying green after the simpler version. That is what we wrote on the guard helper and the compaction line: here is the simpler form, here is how the neighbouring wrappers already solve the same problem, and the evidence that nothing breaks is the suite you already have.
If the claim is about behaviour, the reviewer owes a failing test. The timer fix, good as it was, kept one gap from the old loop: a timer created inside another timer's callback could never fire in the same advanceTime call, although the real process would fire it. Rather than explain that in prose and hope, the review thread carried this:
it('fires a timer scheduled by another timer, when both come due in the same advance', () => {
const proc = ProcessWrapper.createNull();
const order: number[] = [];
proc.startTimer(10, () => {
order.push(10);
proc.startTimer(5, () => order.push(15));
});
proc.advanceTime(100);
expect(order).toEqual([10, 15]);
});
Paste it in, watch it fail, and the claim is no longer the reviewer's opinion. It is a fact about the code that the author can hold in their own hands. The comment then explains why it fails, in this case that the stub advances its clock to the end of the window before any callback runs, so the nested timer anchors its deadline in a future the loop has already passed. And it ends with a hint that points at the mechanism, not the diff: the stub has to move time the way the real clock does, step by step from one due timer to the next. The author writes the code. The test decides when they are done. What came back, and why the fix ran deeper than the one red test, is its own piece, The clock that learned to walk.
Two more habits keep the thread honest. When a finding predates the PR, the comment says so first, because blame is a fact to state and not a feeling to manage; two of our four findings were older than the branch, merely made visible by it. And a thread does not close when the test goes green. It closes when the author writes back what they found, in their own words. Green is the code understanding the change. The sentence is the person understanding it, and the sentence is the one we cannot ship without.
The exit no guard could see
The nested timer was one of the two mirror image threads, the behaviour a test should have watched. The other was the double exit, and it tested this piece's own claim, that a thread closes on the sentence and not the green, harder than anything else on the branch.
The policy arms a deadline, then closes the app: a clean close exits 0, a deadline that fires first exits 1 and stops waiting. But the close that lost the race is still out there, and when it finally settles its handler runs anyway. The original code let it record a clean exit 0 on top of the hard exit 1 already taken. A shutdown that reports it timed out, and then reports it went cleanly. Both cannot be true, and older than the branch, it had stayed quiet because nothing had made it speak.
The author fixed it the honest way, a boolean that remembers an exit already happened, red test first, then green. Every habit in this piece says the thread closes there. It did not, because green was the code agreeing with the change, and the change was not yet the whole design. The flag guarded the two exits it was written beside. A second signal, the operator who means it, leaves on its own line, and that line never touched the flag. So a second red test, pasted into the same thread, ran straight to two exits again:
proc.simulateSignal('SIGTERM');
proc.simulateSignal('SIGTERM'); // exits hard, forgets to remember it
resolveClose(); // the first close lands, a clean 0 on top
The comment named the shape, not the line. This class has four ways out, second signal, deadline, close resolves, close rejects, and the flag was checked at two of them. A guard that lives at each exit is a guard you can forget to add the next time you open one, which is precisely what had happened. So the answer was not a third check. It was to give the question a single home, a small gate the exits pass through instead of each remembering the rule for itself:
private exitOnce(code: number): void {
if (this.exited) {
return;
}
this.exited = true;
this.proc.exit(code);
}
The second signal and the deadline leave through it now, and the rule that used to be copied at each door lives in one. Then the thread closed, on the sentence. The author wrote back, in their own words, what the second signal had been doing before the gate: exiting at once without marking that it had exited, so the still pending close resolved a clean goodbye on top of the hard one. Nobody asked for a recital of the fix. They described the bug as they had come to see it, and that description is the thing no passing suite could have told us.
Set the two ends of the pull request beside each other. It opened with a line nothing could see, a cleanup that ran for no one, and it closed with an exit no guard could see, a way out the flag never covered. The same absence in opposite clothes: one a line the code did not need, the other a line the code did need and no test yet watched for. A refactor's quiet gift is that it moves the furniture enough for both to throw a shadow. Standing where the shadow falls, twice on the same thread if that is what it takes, is the review.
Why we work this way
This is the whole method. Engineers work a real backlog on a real codebase, the review reads the reasoning and not just the diff, and the gap gets taught at the exact moment it costs something, with evidence the author can run rather than authority they have to take on trust. Nobody learns the missing question from a slide. They learn it from a failing test that would not exist if they had asked it.
The move to take home
Refactoring by form says: deduplicate, tighten, clean, align. Refactoring answerable to tests asks one question first: what can observe this line? If the answer is a test, name it, and let it hold the line in place. If the answer is nothing, the line is either missing its test or it does not belong, and the refactor phase is precisely when you are allowed to say which.
And if you review other people's refactors: back every claim with a test. Green suite for claims about shape, a failing test the author can paste in for claims about behaviour. The moment your review can be run instead of believed, it stops being a judgement and becomes a conversation about the code, which is the only conversation that was ever worth having.
