A tool extracts a shared validation routine out of a long handler - what behaviour can silently change?
answer
- Shape matched, semantics never checked
- The happy path was never the risk
- Where does an early exit land now?
- What did the two copies disagree about?
basics
~20 sAn extraction can preserve every happy path and still move the edges: which order side effects run in, whether an early exit still leaves the caller, which failure escapes, and what a missing value defaults to.
solid answer
~50 sA refactor is only a refactor if behaviour is unchanged, and nothing enforces that when the edit comes from a suggestion - it matched the shape of the two blocks, not their semantics. Several things move when a block crosses a boundary: an early rejection now ends the extracted routine instead of the handler, so the caller has to propagate it; a check that used to run only after an earlier one passed may now run on every call, which matters when it has a side effect or can fail; the two blocks were *near*-identical, so the merged routine had to pick one reading of whatever they disagreed about; and the failure that escapes can change type or message. The happy path passes because the happy path was never the risk. Before accepting, ask what the two blocks did differently and pin that difference in a test first.
code
pseudocode · 26 lines# BEFORE - the checks sit inline in the long handler
handleOrder(order):
if order.lines is empty:
audit("empty order", order.id)
return reject("no lines") # leaves handleOrder
if order.currency is missing:
order.currency = accountDefault(order)
charge(order)
return accept(order)
# AFTER - the same statements, lifted into a shared routine
validate(order):
if order.lines is empty:
audit("empty order", order.id)
return reject("no lines") # now leaves validate only
if order.currency is missing:
order.currency = accountDefault(order)
return ok
handleOrder(order):
validate(order) # result discarded
charge(order)
return accept(order)
# The default is still applied, because order is mutated in place.
# The rejection is not: an empty order now reaches charge().go deeper
Remember the promise: a refactor leaves behaviour unchanged. Be able to say that moving code can break that promise, and that the tests you already have only cover the cases someone already thought of.
Explain the mechanics: where an early exit lands once the block moves, when a check that used to be skipped starts running, and what a merge has to decide when two blocks are only nearly the same.
Show the working order - pin the edge case in a test before accepting, diff the two original blocks against each other, and review the call sites rather than the extracted body.
Own the separation: a restructuring that also changes behaviour is two commits, because each half needs different evidence and a mixed commit gets reviewed as whichever half looks smaller.
## The promise a refactor makes A refactor is a change of structure that leaves observable behaviour alone: the same inputs produce the same outputs and the same side effects, in the same order, with the same failures. Nothing enforces that promise when the edit arrives as a suggestion. The tool produced a plausible rewrite, not a proof, and what it matched was the **shape** of the code it was shown. Shape and semantics agree comfortably in the middle of a block and part company at its edges — which is exactly where a long handler keeps the special cases it has accumulated. ## The situation this question comes from Two blocks near the top of a two-thousand-line order handler run nearly the same checks. Lifting them into one `validate` routine is the textbook move: less duplication, a name for the concept, a shorter handler. The suggestion arrives as one tidy diff — a new routine, two call sites replaced — and the suite stays green. ## Where the behaviour actually moved 1. **The escape route.** If the lifted block contained an early rejection, that exit used to leave the handler. Inside a routine it leaves the routine. Unless the call site inspects the result and propagates it, the rejected input walks on into the work the rejection existed to prevent. In the diff this is invisible: the body is character-for-character what it was. 2. **Order and short-circuiting.** A check that ran only after an earlier one passed may now run on every call, or the reverse. Where a check has a side effect — an audit line, a counter, a lock, a value built on first use — or can fail, the set of inputs that reach it has changed even though its code has not. 3. **The merged difference.** *Near*-identical is the risk, not the duplication. If the two blocks disagreed about one bound, one default or one extra check, the merged routine has to pick one reading, and a suggestion picks whichever reading looked more natural. One call site quietly gains a rule; the other quietly loses one. 4. **What escapes on failure.** An extraction often changes what leaves the boundary when something goes wrong: a different failure type, a different message, or the original wrapped in something new. A caller that branches on the type, or an alert keyed to the text, is behaviour too. 5. **What the caller used to fill in.** A default supplied at the call site before the block ran may now be supplied inside the routine, after a check that previously saw the raw value. | what moved | how it looks in the diff | what actually catches it | |---|---|---| | An exit that no longer exits | an unchanged body plus a new call | a test asserting the rejected input causes no effect | | A check whose reachability changed | reordered lines, or nothing at all | a test on the input that used to skip it | | Two readings merged into one rule | a single new routine | reading the two originals against each other | | A different failure escaping | a new raise or a new wrap | a test asserting which failure, not merely that it failed | ## Why the suite stayed green anyway Tests cover the cases somebody thought of, and the happy path is the case everybody thinks of. The rejection branch is often asserted at the wrong level: the check returns the right verdict, so the assertion passes, while nothing asserts that the order was never charged. A green run after a structural edit tells you the covered behaviour survived. It does not tell you behaviour survived. That asymmetry is what makes this class of mistake expensive — it ships, it is not attributed to the refactor that caused it, and it surfaces days later in data rather than in a stack trace. ## Making the difference visible before you accept 1. Say what you claim to preserve, in one sentence, before you look at the diff. "An order with no lines is rejected and never charged" is checkable; "it behaves the same" is not. 2. Diff the two original blocks **against each other**, not against the new routine. The suggestion's job was to merge them, and everything it had to decide lives in what they disagreed about. 3. Pin the edge case in a test that fails if the behaviour moves, and run it **before** the edit. A test written afterwards describes the new behaviour, including the part you were trying to detect. 4. Review the **call sites**, not the extracted body. The body is the part that did not change. 5. Keep any behaviour change out of the commit. If the merged rule really is better, that is a second change with its own test and its own argument. ## What a good answer sounds like Name the mechanism rather than an anecdote: an extraction moves code across a boundary, and a boundary is where control flow, evaluation order, defaults and failures are all renegotiated. The tool is dependable about the text and has no way to know, from the code alone, which of the two readings you meant. So the safe version of the move is small, pinned by a test that predates it, and reviewed from the outside in.
- The two blocks you merged differed in one check. What do you do with that difference?Decide it explicitly rather than letting the merge decide it. Either the difference is real, in which case it becomes a parameter or the blocks stay separate, or it was a bug, in which case fixing it is a second commit with its own test. What it must not be is a silent choice inside a refactor.
- Which tests would catch a dropped early exit, and which would not?Only a test that asserts the effect of the rejected path - nothing charged, nothing persisted, nothing sent. A test asserting that the validation returns a rejection still passes, because the validation still returns one; what changed is that nobody acts on it.
- How do you keep this from depending on your own alertness every time?Shrink what has to be noticed. Accept extractions only where a test already pins the edge behaviour, keep the commit to the mechanical move alone, and review the call sites rather than the body, so the surface you must read is small enough to actually read.
Moving a block into its own routine is like moving a paragraph out of a letter and into an enclosed leaflet. Every word survives, but the sentence that said stop reading here now only stops the leaflet.
saying these in an interview costs you the question
- Says a green suite proves the extraction preserved behaviour
- Calls the extraction safe because the body was copied unchanged
- Reviews only the new routine, never the call sites it replaced
- Assumes two near-identical blocks must have had identical intent
- Lets a behaviour change ride along inside a refactor commit