Refactoring — code smells, common refactorings (extract method, rename, etc.): Question bank

This bank drills the Refactoring topic — specifically the boundaries: what counts as refactoring, what secretly changes behaviour, and when a "clean" move is actually harmful.

Definition Jargon defined up-front (used below)

==God Object== — one class that hoards far too many unrelated responsibilities, so it "knows about" everything and every change touches it.
==Shotgun Surgery== — a smell where making one logical change forces edits scattered across many files/classes, like buckshot.
==Design knot== — a spot where two changes are so entangled you cannot make one without the other; there is no smaller safe step.
==Wedge== — a tiny intermediate refactoring that pries a knot apart into two independently-safe steps (e.g. add a new path alongside the old, migrate, then delete the old).
==Constant folding / inlining== — compiler optimisations that compute constant expressions at build time or paste a value/function body in place; they can make "identical" source differ in emitted machine code.
==Single Responsibility Principle (SRP)== — the design rule that a class/module should have one reason to change, i.e. own exactly one responsibility. A God Object breaks this by owning many. See Single Responsibility Principle.
==DRY Principle ("Don't Repeat Yourself")== — every piece of knowledge should live in one place, so a change is made once, not copy-pasted. Duplicated Code violates it. See DRY Principle.
==Cyclomatic Complexity== — a count of the independent paths through a function (roughly, one plus the number of branches like if/for/and). Higher = more paths to test and harder to reason about. See Cyclomatic Complexity.
==Unit test / characterization test== — a small automated check that asserts a piece of code gives an expected output for a given input; a characterization test pins down whatever behaviour code currently has so you can refactor without changing it. See Unit Testing.
==Clean Code== — the practice of writing code that reads like prose: names reveal intent, functions do one thing, comments explain why not what. See Clean Code.
==Design pattern== — a named, reusable solution shape for a recurring design problem (e.g. Strategy = swap one algorithm for another behind a common interface). See Design Patterns.

The transformations at a glance (visual-first)

Before drilling traps, anchor the two moves the bank keeps referencing.

Figure — Refactoring — code smells, common refactorings (extract method, rename, etc.)

Alt text / walkthrough. Top row = Extract Method. The pink box on the left is print_invoice doing three jobs at once (print a header, loop-and-sum the items, and multiply by 1.0825). Follow the yellow arrow rightward: the tangle splits into a blue box (print_invoice now only prints and calls something) plus a small yellow box (compute_total(), the concept we lifted out and named). Bottom row = Extract Class. The left pink box shows distance and midpoint each dragging the same four loose numbers x1,y1,x2,y2 (a data clump). The yellow arrow leads to a blue box where both functions now take just (a, b), and the yellow box underneath is the new Point(x, y) class that names the hidden concept. In both rows: pink = the messy "before", yellow = the newly named thing, blue = the simplified caller.

Alt text / walkthrough. Read the top row left-to-right as three gated stages: blue "Cover with passing tests" → (white arrow) → yellow "One tiny refactor step" → (white arrow) → pink "Run tests". From "Run tests" two coloured arrows branch downward: the blue arrow to "GREEN → commit" (keep the change) and the pink arrow to "RED → revert" (throw it away instantly). The long white arrow curving from the green box back up to the first step is the repeat loop — you circle here forever, one tiny verified step at a time. The yellow caption at the bottom states the invariant that must hold at every stage: behaviour stays identical.

Worked example Side-by-side: Extract Method (the canonical transformation)

Before — one method mixing "print" with "compute tax":

def print_invoice(order):
    print("=== Invoice ===")
    total = 0
    for item in order.items:
        total += item.price * item.qty
    total *= 1.0825          # magic number + hidden concept
    print(f"Total: ${total:.2f}")

After — the concept is named, reusable, and independently testable:

TAX_RATE = 0.0825
 
def compute_total(order):
    subtotal = sum(i.price * i.qty for i in order.items)
    return subtotal * (1 + TAX_RATE)
 
def print_invoice(order):
    print("=== Invoice ===")
    print(f"Total: ${compute_total(order):.2f}")

Same number printed → behaviour preserved. Structure improved → this is a refactoring.

Worked example Side-by-side: Extract Class (data clump → object)

Before — four coordinates travel together everywhere (a data clump + long parameter list):

def distance(x1, y1, x2, y2): ...
def midpoint(x1, y1, x2, y2): ...

After — the hidden concept Point is named once:

class Point:
    def __init__(self, x, y): self.x, self.y = x, y
 
def distance(a: Point, b: Point): ...
def midpoint(a: Point, b: Point): ...

True or false — justify

A rename that changes a public API method name is still a pure refactoring.

False, because refactoring must preserve observable behaviour and an external caller who invoked the old name now gets an error — the observable behaviour for that caller changed. That makes it a breaking change, so it needs a major-version bump under semantic versioning (in X.y.z the leading X increments), or a deprecation window where you keep the old name as a delegating alias, emit a deprecation warning, and remove it only in a later major release.

Once you add a deprecated alias for a renamed public method, you can delete the old name in the next patch release.

False, because semantic versioning defines patch (z) and minor (y) releases as non-breaking; removing a public name breaks any caller still using it, which is a breaking change reserved for the next major (X) bump — and only after the deprecation period gave consumers time to migrate.

Refactoring can be done safely without any tests as long as the change is tiny.

False, because "tiny" edits silently flip <= to < or reorder side effects, and with no tests you have no evidence the outputs stayed the same. The tests are your proof that behaviour was preserved, and that proof is the very definition of a refactoring — a unit test (see Unit Testing, an automated input→output check) is what turns "I think it's safe" into "I can show it's safe".

Fixing a bug while cleaning up the surrounding code counts as one refactoring.

False, because fixing a bug changes observable behaviour (a wrong output becomes right), which is the opposite of behaviour-preserving. Since the two goals conflict, refactor and bug-fix belong in separate commits so a failing test points at exactly one cause.

A code smell always means there is a bug in the code.

False, because a smell is defined as a hint of a deeper design problem, not a defect — the mechanism it flags is future pain, not present wrongness. The code may pass every test and ship correctly; the smell only warns that the next change will be harder.

Extracting a method never changes performance.

False, because an extra function call adds (usually tiny) call overhead, and careless extraction can change when or how often a value is computed. Behaviour (the outputs) is preserved, but timing and cost are not part of "behaviour", so they can shift.

Adding tests to code that had none is itself a refactoring.

False, because refactoring is a transformation of production code, and adding tests leaves that code's structure and behaviour untouched. It's the enabling prerequisite (it gives you the proof net), not the transformation itself.

Renaming a variable is the lowest-value refactoring because it changes so little.

False, because value is measured per unit of effort and a good name pays off on every future read. A bad name forces each reader to reconstruct meaning from scratch; good names are documentation that never goes stale (the naming-conventions idea of intent-revealing names).

If all tests pass after a change, the change was definitely a valid refactoring.

False, because tests only sample behaviour — they are a proxy, not the whole of it. If the suite doesn't exercise some path, you can alter that uncovered behaviour and still see green, so passing tests bound your certainty rather than guarantee it.

Replacing a magic number 0.0825 with TAX_RATE = 0.0825 changes the program's observed output.

False for the computed result, because subtotal * (1 + 0.0825) and subtotal * (1 + TAX_RATE) evaluate to the identical number. Caveat: the emitted machine code need not be identical — a compiler may constant-fold the inline literal but load TAX_RATE from memory (or fold it too), so a byte-for-byte identical binary is not guaranteed; only the behaviour is.

More, shorter methods is always cleaner than fewer, longer ones.

False, because extraction only helps when it names a concept; splitting for its own sake creates indirection where you bounce through many files to follow one idea. So the honest measure is clarity, not line count — over-extraction actively hurts readability.

Spot the error

"I refactored the login flow and also made it reject empty passwords — much cleaner now."

The error is mixing hats: rejecting empty passwords is a behaviour change (new outputs for empty input), not refactoring. Split it into a refactor commit and a feature commit so a broken test blames only one.

"My extract-method left a stray total *= 1.0825 in the caller and in the new function, so tax is applied twice, but the code looks tidier."

Behaviour changed — the total is now wrong (double tax). Tidier structure with a different output is a bug, not a refactoring; tests should have caught the doubled multiplication.

"I introduced an explaining variable but wrote is_adult = user.age > 18 where the original was >= 18."

The > vs >= flip changes the truth value for age exactly 18, so an explaining variable that was supposed to be a pure rename silently altered a boundary case. This is why boundary tests matter.

"To fix Duplicated Code I copied the shared logic into a third place so all three copies match."

That makes duplication worse — now three copies drift out of sync. The fix is to extract the logic to one location all callers share, per the DRY Principle (keep each piece of knowledge in exactly one place).

"I added a comment // adds 8.25% tax above a cryptic multiply and called it done."

A comment explaining what obscure code does is usually a smell used as deodorant. Prefer extracting a named method/constant so the code self-documents; save comments for why. This is the Clean Code stance: names carry intent, comments carry rationale.

"I merged two small classes into one big class because jumping between files annoyed me."

You created a God Object — a class hoarding multiple responsibilities — violating the Single Responsibility Principle (one class, one reason to change). Fewer files isn't the goal; clear responsibilities are.

"I extracted counter += 1 into a helper method inside a multi-threaded server; the field was guarded by the caller's synchronized block."

The extraction may have moved the increment outside the lock's scope (or into a separately-locked method), so two threads can now interleave and lose updates. In concurrent code, extraction can change synchronization boundaries and side-effect ordering even though the single-threaded output looks identical.

Why questions

Why must behaviour stay fixed for a change to count as refactoring?

Because if structure and behaviour change together and a test fails, you can't tell which change broke it. Freezing behaviour turns refactoring into a controlled experiment where any failure points straight at your structural mistake.

Why are tests called a "proxy" for behaviour rather than behaviour itself?

Because behaviour is every possible input→output, while tests only sample some of those. They approximate behaviour, so the closer the suite matches real usage, the safer the refactor — poor tests mean unsafe refactoring.

Why does the "Rule of Three" wait until the third copy before extracting?

Because one copy is fine; a second might be coincidence and premature abstraction is costly to undo. The third occurrence confirms a real, stable pattern worth naming — copy once = fine, twice = wince, thrice = refactor.

Why does Extract Method make code more testable, not just more readable?

Because the extracted fragment becomes an independent, named unit you can call and assert on directly, instead of only being reachable through the larger method's side effects. See Unit Testing (small, direct input→output checks).

Why is a Data Clump (like x1, y1, x2, y2 travelling together) a signal to Extract Class?

Because the repeated group is secretly a hidden concept (two Points). Naming that structure removes both the clump and the long parameter list at once, and centralizes any logic about points.

Why does unpaid technical debt make future changes slower and riskier?

Because small messes compound; each new feature must navigate the tangle, and unclear structure hides bugs. Refactoring pays the debt down before the "interest" (bugs, slowdowns) accumulates. See Technical Debt (the future cost of today's shortcuts).

Why can a very long method with high branching be dangerous even if it passes today's tests?

Because high Cyclomatic Complexity (many independent paths) means it's easy to leave one path untested and hard to reason about the whole. It works now but resists safe change later.

Why is a public-API rename fundamentally harder than a private-variable rename?

Because a private name has a closed, known set of references you fully control, whereas a public name has unknown external callers you cannot reach. You therefore cannot update them all at once — hence semantic-versioning bumps and deprecation aliases instead of an instant rename.

Edge cases

Is deleting genuinely dead (unreachable) code a refactoring?

Yes — if the code can never execute, removing it cannot change any observable output, so behaviour is preserved. The subtlety is proving it's truly unreachable (reflection, dynamic dispatch can fool you).

You refactor a function that has no tests but is "obviously trivial." What's the danger?

With zero tests you have no proof behaviour was preserved, so by definition it isn't a safe refactoring — it's an unverified edit. Write a characterization test first (one that pins the current output), then refactor.

A rename tool renames a variable but misses one occurrence built from a string at runtime. Behaviour-preserving?

No — the runtime-constructed name still points at the old identifier, so the program breaks or diverges. Dynamic/reflective references are the classic case where an "automatic" rename silently changes behaviour.

Two code fragments look identical but are duplicated for different reasons that will evolve separately. Should you DRY them?

Not necessarily — merging them couples two concepts that must change independently, creating Shotgun Surgery later (one logical change scattered across many edits). Duplication is only a smell when the copies represent the same concept.

Is reformatting whitespace / reordering imports a refactoring?

Debatably it's the most trivial kind — it preserves behaviour but doesn't improve structure, only cosmetics. Most people call it "formatting," reserving "refactoring" for changes that improve design, though it's harmless to the behaviour invariant.

What happens at the boundary where a "one tiny step" refactor is actually two coupled changes you can't separate (a design knot)?

You've hit a design knot — no smaller safe step exists. Introduce a wedge: a tiny intermediate refactoring (e.g. add the new structure alongside the old, migrate callers, then delete the old) that splits the knot into two independently-testable moves.

Does extracting a method in concurrent code preserve behaviour?

Not automatically — if the extracted fragment was inside a lock, transaction, or relied on ordered side effects, moving it can change which operations are atomic together. Single-threaded outputs may match while a race is introduced, so concurrency needs its own tests and careful lock-scope preservation.

If a refactoring introduces a design pattern (e.g. Strategy) but the outputs are unchanged, is it still refactoring?

Yes — introducing a pattern to improve structure while keeping identical observable behaviour is a textbook refactoring. The trap is adding the pattern speculatively for behaviour you don't yet need: that just piles on indirection and complexity for no current benefit, so only reach for a pattern once a real, recurring problem justifies it.

Recall One-line self-test before you close

If you cannot name a test that would go red if your change broke something, you were not refactoring — you were gambling.