Bad Structure Anti-Patterns — Refactoring Practice¶

Category: Development Anti-Patterns → Bad Structure — code that has grown into a shape that resists change. Covers (collectively): God Object · Spaghetti Code · Lava Flow · Boat Anchor · Arrow Anti-Pattern

These are not "spot the smell" puzzles — find-bug.md does that. Here the code is structurally bad but already working, and your job is to transform it into clean code without changing behavior. The skill on display is the process, not the destination:

1. Pin behavior first. Write a characterization test — a test that records what the code does now, bugs included. You are freezing behavior, not blessing it.
2. Take small, reversible steps. One named refactoring at a time (Extract Method, Extract Class, …), tests green after each, commit.
3. Separate structural from behavioral commits. A refactor must preserve behavior; if a test goes red, the last step is the culprit — revert it.

Each worked solution names the canonical refactoring moves (from Fowler's Refactoring and Feathers' Working Effectively with Legacy Code) so you build the vocabulary, not just the instinct.

How to use this file: read the "Before" code, write down the move sequence yourself before expanding the solution, then compare. The gap between your plan and the worked plan is where the learning is.

Table of Contents¶

Exercise 1 — Flatten the validation tower¶

Anti-pattern: Arrow. Goal: flatten the nesting so the happy path is obvious. Constraints: identical return values for every input; no behavior change.

// Before — Arrow: the success line is 5 indents deep.
func Withdraw(acc *Account, amount int) (int, error) {
    if acc != nil {
        if !acc.Frozen {
            if amount > 0 {
                if amount <= acc.Balance {
                    if acc.DailyWithdrawn+amount <= acc.DailyLimit {
                        acc.Balance -= amount
                        acc.DailyWithdrawn += amount
                        return acc.Balance, nil
                    } else {
                        return 0, errors.New("daily limit exceeded")
                    }
                } else {
                    return 0, errors.New("insufficient funds")
                }
            } else {
                return 0, errors.New("amount must be positive")
            }
        } else {
            return 0, errors.New("account frozen")
        }
    }
    return 0, errors.New("nil account")
}

// After — guard clauses; happy path is flat and last.
func Withdraw(acc *Account, amount int) (int, error) {
    if acc == nil {
        return 0, errors.New("nil account")
    }
    if acc.Frozen {
        return 0, errors.New("account frozen")
    }
    if amount <= 0 {
        return 0, errors.New("amount must be positive")
    }
    if amount > acc.Balance {
        return 0, errors.New("insufficient funds")
    }
    if acc.DailyWithdrawn+amount > acc.DailyLimit {
        return 0, errors.New("daily limit exceeded")
    }

    acc.Balance -= amount
    acc.DailyWithdrawn += amount
    return acc.Balance, nil
}

Exercise 2 — Kill the boolean-flag function¶

Anti-pattern: Spaghetti (flag-driven). Goal: remove the boolean parameters so each behavior has its own clear entry point. Constraints: preserve all four current behaviors; callers may be updated.

// Before — three booleans = up to 8 paths in one body.
void notify(User user, boolean isUrgent, boolean asSms, boolean dryRun) {
    String channel = asSms ? "sms" : "email";
    String prefix  = isUrgent ? "[URGENT] " : "";
    String body    = prefix + render(user);

    if (dryRun) {
        log.info("would send via {} to {}: {}", channel, user.contact(channel), body);
        return;
    }
    if (asSms) {
        sms.send(user.phone(), body);
    } else {
        email.send(user.email(), body, isUrgent ? Priority.HIGH : Priority.NORMAL);
    }
}

// After — intent is in the function name, not in a boolean soup.
String buildBody(User user, boolean urgent) {
    return (urgent ? "[URGENT] " : "") + render(user);
}

void notifyEmail(User user, Priority priority) {
    email.send(user.email(), buildBody(user, priority == Priority.HIGH), priority);
}

void notifySms(User user, boolean urgent) {
    sms.send(user.phone(), buildBody(user, urgent));
}

// dry-run is a cross-cutting concern, not a behavior flag:
void previewEmail(User user, Priority priority) {
    log.info("would email {}: {}", user.email(), buildBody(user, priority == Priority.HIGH));
}

Exercise 3 — Extract a method from a long block¶

Anti-pattern: Spaghetti (Long Method). Goal: decompose a 40-line procedure into named steps. Constraints: same return value; same side effects in the same order.

# Before — one function doing parse + validate + transform + summarize.
def process_orders(rows):
    total = 0
    valid = []
    for r in rows:
        parts = r.strip().split(",")
        if len(parts) != 3:
            continue
        oid, qty_s, price_s = parts
        if not qty_s.isdigit():
            continue
        qty = int(qty_s)
        try:
            price = float(price_s)
        except ValueError:
            continue
        if qty <= 0 or price <= 0:
            continue
        line_total = qty * price
        if line_total > 10000:
            line_total *= 0.95  # bulk discount
        total += line_total
        valid.append({"id": oid, "qty": qty, "total": round(line_total, 2)})
    return {"orders": valid, "grand_total": round(total, 2)}

# After — each step is named and independently testable.
def process_orders(rows):
    valid, total = [], 0.0
    for r in rows:
        parsed = parse_row(r)
        if parsed is None:
            continue
        oid, qty, price = parsed
        amount = line_total(qty, price)
        total += amount
        valid.append({"id": oid, "qty": qty, "total": round(amount, 2)})
    return {"orders": valid, "grand_total": round(total, 2)}

def parse_row(r):
    parts = r.strip().split(",")
    if len(parts) != 3:
        return None
    oid, qty_s, price_s = parts
    if not qty_s.isdigit():
        return None
    try:
        price = float(price_s)
    except ValueError:
        return None
    qty = int(qty_s)
    if qty <= 0 or price <= 0:
        return None
    return oid, qty, price

def line_total(qty, price):
    total = qty * price
    return total * 0.95 if total > 10000 else total  # bulk discount

Exercise 4 — Delete the boat anchor¶

Anti-pattern: Boat Anchor. Goal: remove an abstraction built "for the future" that has zero real call sites. Constraints: no observable behavior change; don't break the build.

// Before — a 3-format exporter interface; only PDF is ever called.
type ReportExporter interface {
    ExportPDF(r Report) ([]byte, error)
    ExportXLSX(r Report) ([]byte, error)
    ExportXML(r Report) ([]byte, error) // no caller, ever
}

type exporter struct{ tmpl *template.Template }

func (e *exporter) ExportPDF(r Report) ([]byte, error)  { /* used */ return renderPDF(e.tmpl, r) }
func (e *exporter) ExportXLSX(r Report) ([]byte, error) { /* 90 lines, no caller */ }
func (e *exporter) ExportXML(r Report) ([]byte, error)  { /* 70 lines, no caller */ }

grep -rn "ExportXLSX\|ExportXML" --include="*.go" .
go run golang.org/x/tools/cmd/deadcode@latest ./...

// After — only the capability that exists is kept.
func ExportPDF(tmpl *template.Template, r Report) ([]byte, error) {
    return renderPDF(tmpl, r)
}

Exercise 5 — Prove and remove lava flow¶

Anti-pattern: Lava Flow. Goal: safely delete a fossilized branch nobody understands. Constraints: you must produce evidence it's dead before deleting; behavior unchanged for live paths.

# Before — a scary branch with a fear-comment. Is it alive?
def price_quote(cart, region):
    base = sum(i.price * i.qty for i in cart.items)

    # DO NOT REMOVE — legacy promo path, broke prod in 2019?? -- unknown
    if region == "LEGACY_EU" and cart.coupon == "SPRING07":
        base = _spring_2007_promo(base, cart)   # 120 lines, no docs

    if cart.coupon == "WELCOME":
        base *= 0.9
    return round(base, 2)

if region == "LEGACY_EU" and cart.coupon == "SPRING07":
    metrics.increment("legacy.spring07_promo.hit")   # temporary probe
    base = _spring_2007_promo(base, cart)

# After — fossil removed; live discount path untouched.
def price_quote(cart, region):
    base = sum(i.price * i.qty for i in cart.items)
    if cart.coupon == "WELCOME":
        base *= 0.9
    return round(base, 2)

Exercise 6 — Replace the type-switch arrow¶

Anti-pattern: Arrow + Spaghetti (type-switching). Goal: remove the instanceof ladder. Constraints: same rendered output; adding a new shape must not require editing the renderer.

// Before — Arrow by type-switch; every new shape adds a branch.
String describe(Shape s) {
    if (s instanceof Circle) {
        Circle c = (Circle) s;
        if (c.radius() > 0) {
            return "circle area=" + (Math.PI * c.radius() * c.radius());
        } else {
            return "degenerate circle";
        }
    } else if (s instanceof Rectangle) {
        Rectangle r = (Rectangle) s;
        if (r.width() > 0 && r.height() > 0) {
            return "rect area=" + (r.width() * r.height());
        } else {
            return "degenerate rect";
        }
    } else if (s instanceof Triangle) {
        Triangle t = (Triangle) s;
        return "triangle area=" + (0.5 * t.base() * t.height());
    }
    throw new IllegalArgumentException("unknown shape");
}

// After — each type owns its behavior; renderer never changes again.
interface Shape {
    String describe();
}

final class Circle implements Shape {
    private final double radius;
    Circle(double radius) { this.radius = radius; }
    public String describe() {
        return radius > 0 ? "circle area=" + (Math.PI * radius * radius)
                          : "degenerate circle";
    }
}

final class Rectangle implements Shape {
    private final double width, height;
    Rectangle(double w, double h) { this.width = w; this.height = h; }
    public String describe() {
        return width > 0 && height > 0 ? "rect area=" + (width * height)
                                       : "degenerate rect";
    }
}

final class Triangle implements Shape {
    private final double base, height;
    Triangle(double b, double h) { this.base = b; this.height = h; }
    public String describe() { return "triangle area=" + (0.5 * base * height); }
}

// caller:
String out = shape.describe();

Exercise 7 — Table-driven dispatch¶

Anti-pattern: Arrow (value-based dispatch). Goal: replace a string-switch arrow with a handler table. Constraints: identical results; unknown command still errors.

// Before — a growing switch ladder over command names.
func Handle(cmd string, args []string) (string, error) {
    if cmd == "add" {
        return strconv.Itoa(atoi(args[0]) + atoi(args[1])), nil
    } else if cmd == "sub" {
        return strconv.Itoa(atoi(args[0]) - atoi(args[1])), nil
    } else if cmd == "mul" {
        return strconv.Itoa(atoi(args[0]) * atoi(args[1])), nil
    } else if cmd == "neg" {
        return strconv.Itoa(-atoi(args[0])), nil
    } else {
        return "", fmt.Errorf("unknown command: %s", cmd)
    }
}

// After — adding a command = adding one map entry.
type handler func(args []string) (string, error)

var handlers = map[string]handler{
    "add": func(a []string) (string, error) { return strconv.Itoa(atoi(a[0]) + atoi(a[1])), nil },
    "sub": func(a []string) (string, error) { return strconv.Itoa(atoi(a[0]) - atoi(a[1])), nil },
    "mul": func(a []string) (string, error) { return strconv.Itoa(atoi(a[0]) * atoi(a[1])), nil },
    "neg": func(a []string) (string, error) { return strconv.Itoa(-atoi(a[0])), nil },
}

func Handle(cmd string, args []string) (string, error) {
    h, ok := handlers[cmd]
    if !ok {
        return "", fmt.Errorf("unknown command: %s", cmd)
    }
    return h(args)
}

Exercise 8 — Introduce a parameter object¶

Anti-pattern: Spaghetti (long, order-sensitive parameter lists). Goal: group a clump of always-together parameters. Constraints: preserve computed result; reduce the chance of arg-order bugs.

# Before — six positional params; easy to transpose at the call site.
def shipping_cost(weight, length, width, height, dest_zone, express):
    volume = length * width * height
    dim_weight = volume / 5000
    billable = max(weight, dim_weight)
    rate = 2.0 if dest_zone == "domestic" else 5.0
    cost = billable * rate
    if express:
        cost *= 1.5
    return round(cost, 2)

# call site (which one is width vs height again?)
shipping_cost(3.2, 40, 30, 20, "intl", True)

# After — cohesive value object; the call site is self-describing.
from dataclasses import dataclass

@dataclass(frozen=True)
class Parcel:
    weight: float
    length: float
    width: float
    height: float

    @property
    def volume(self) -> float:
        return self.length * self.width * self.height

    @property
    def billable_weight(self) -> float:
        return max(self.weight, self.volume / 5000)

def shipping_cost(parcel: Parcel, dest_zone: str, express: bool) -> float:
    rate = 2.0 if dest_zone == "domestic" else 5.0
    cost = parcel.billable_weight * rate
    if express:
        cost *= 1.5
    return round(cost, 2)

# call site — impossible to transpose length/width silently:
shipping_cost(Parcel(weight=3.2, length=40, width=30, height=20), "intl", express=True)

Exercise 9 — Split the God Object¶

Anti-pattern: God Object. Goal: split a class with three unrelated field-clusters into cohesive collaborators, wired by dependency injection. Constraints: preserve the public checkout() behavior and its side effects; callers of checkout() must not change.

// Before — one class does pricing, payment, and notification.
public class OrderManager {
    private final Map<String,Double> priceList;
    private final String stripeKey;
    private final SmtpClient smtp;

    public OrderManager(Map<String,Double> priceList, String stripeKey, SmtpClient smtp) {
        this.priceList = priceList; this.stripeKey = stripeKey; this.smtp = smtp;
    }

    public Receipt checkout(Order order) {
        // pricing
        double subtotal = 0;
        for (Item i : order.items()) subtotal += priceList.get(i.sku()) * i.qty();
        double tax = subtotal * 0.08;
        double total = subtotal + tax;

        // payment
        String charge = Stripe.charge(stripeKey, order.card(), (long)(total * 100));

        // notification
        smtp.send(order.email(), "Receipt", "You paid $" + total + " (charge " + charge + ")");

        return new Receipt(order.id(), total, charge);
    }
}

// After — three cohesive services, one thin coordinator.
public class PricingService {
    private final Map<String,Double> priceList;
    public PricingService(Map<String,Double> priceList) { this.priceList = priceList; }
    public double total(Order order) {
        double subtotal = 0;
        for (Item i : order.items()) subtotal += priceList.get(i.sku()) * i.qty();
        return subtotal + subtotal * 0.08; // tax
    }
}

public class PaymentGateway {
    private final String stripeKey;
    public PaymentGateway(String stripeKey) { this.stripeKey = stripeKey; }
    public String charge(Order order, double total) {
        return Stripe.charge(stripeKey, order.card(), (long)(total * 100));
    }
}

public class ReceiptNotifier {
    private final SmtpClient smtp;
    public ReceiptNotifier(SmtpClient smtp) { this.smtp = smtp; }
    public void send(Order order, double total, String charge) {
        smtp.send(order.email(), "Receipt", "You paid $" + total + " (charge " + charge + ")");
    }
}

public class OrderManager {
    private final PricingService pricing;
    private final PaymentGateway payment;
    private final ReceiptNotifier notifier;

    public OrderManager(PricingService pricing, PaymentGateway payment, ReceiptNotifier notifier) {
        this.pricing = pricing; this.payment = payment; this.notifier = notifier;
    }

    public Receipt checkout(Order order) {           // public API unchanged
        double total  = pricing.total(order);
        String charge = payment.charge(order, total);
        notifier.send(order, total, charge);
        return new Receipt(order.id(), total, charge);
    }
}

Exercise 10 — Untangle shared mutable state¶

Anti-pattern: Spaghetti (global state, order-dependent calls). Goal: convert hidden-shared-state functions into an explicit pipeline. Constraints: same final output; functions must be callable independently.

# Before — a module-level dict; functions must run in a secret order.
_ctx = {}

def load(path):
    _ctx["raw"] = open(path).read()

def parse():
    _ctx["rows"] = [l.split(",") for l in _ctx["raw"].splitlines()]  # KeyError if load() didn't run

def filter_valid():
    _ctx["rows"] = [r for r in _ctx["rows"] if len(r) == 3]

def total():
    return sum(float(r[2]) for r in _ctx["rows"])

def run(path):
    load(path); parse(); filter_valid()
    return total()

# After — explicit pipeline; each function is pure and independently callable.
def load(path):
    with open(path) as f:
        return f.read()

def parse(raw):
    return [line.split(",") for line in raw.splitlines()]

def filter_valid(rows):
    return [r for r in rows if len(r) == 3]

def total(rows):
    return sum(float(r[2]) for r in rows)

def run(path):
    raw   = load(path)
    rows  = filter_valid(parse(raw))
    return total(rows)

Exercise 11 — Spaghetti → explicit state machine¶

Anti-pattern: Spaghetti (scattered status flags). Goal: replace boolean-flag soup that encodes a lifecycle with an explicit state machine. Constraints: the same transitions are legal/illegal as before; same errors on illegal transitions.

// Before — order lifecycle smeared across booleans; invalid combos are representable.
type Order struct {
    Paid      bool
    Shipped   bool
    Delivered bool
    Cancelled bool
}

func (o *Order) Pay() error {
    if o.Cancelled { return errors.New("cancelled") }
    if o.Paid      { return errors.New("already paid") }
    o.Paid = true
    return nil
}

func (o *Order) Ship() error {
    if o.Cancelled        { return errors.New("cancelled") }
    if !o.Paid            { return errors.New("not paid") }
    if o.Shipped          { return errors.New("already shipped") }
    o.Shipped = true
    return nil
}

func (o *Order) Cancel() error {
    if o.Delivered { return errors.New("already delivered") }
    if o.Shipped   { return errors.New("already shipped") }   // can't cancel once shipped
    o.Cancelled = true
    return nil
}
// (Deliver omitted) — note: {Paid:false, Shipped:true} is representable but illegal.

// After — one State; a table defines the only legal transitions.
type State int

const (
    Created State = iota
    Paid
    Shipped
    Delivered
    Cancelled
)

// allowed[from] = set of states reachable in one step
var allowed = map[State]map[State]bool{
    Created: {Paid: true, Cancelled: true},
    Paid:    {Shipped: true, Cancelled: true},
    Shipped: {Delivered: true},
    // Delivered, Cancelled: terminal
}

type Order struct{ State State }

func (o *Order) transition(to State, illegal error) error {
    if !allowed[o.State][to] {
        return illegal
    }
    o.State = to
    return nil
}

func (o *Order) Pay() error      { return o.transition(Paid, errors.New("cannot pay")) }
func (o *Order) Ship() error     { return o.transition(Shipped, errors.New("cannot ship")) }
func (o *Order) Deliver() error  { return o.transition(Delivered, errors.New("cannot deliver")) }
func (o *Order) Cancel() error   { return o.transition(Cancelled, errors.New("cannot cancel")) }

Exercise 12 — Strangler-fig a God Object module¶

Anti-pattern: God Object + Lava Flow. Goal: carve a focused module out of a large legacy class incrementally, while it stays in production. Constraints: zero downtime; old and new must coexist; each step independently shippable.

# Before — a 1,500-line LegacyBilling class; we only need to extract tax logic safely.
class LegacyBilling:
    def invoice(self, account, lines):
        # ... 200 lines of intertwined discount + tax + ledger code ...
        tax = 0.0
        for ln in lines:
            rate = 0.08 if account.region == "US" else 0.20
            if ln.category == "food":         # lava-flow special-case from 2014
                rate = 0.0
            tax += ln.amount * rate
        # ... more intertwined code uses `tax` ...
        return self._assemble(account, lines, tax)

class TaxCalculator:
    def compute(self, account, lines):
        rate_for = lambda ln: 0.0 if ln.category == "food" \
                              else (0.08 if account.region == "US" else 0.20)
        return sum(ln.amount * rate_for(ln) for ln in lines)

class LegacyBilling:
    def __init__(self, tax: TaxCalculator | None = None, use_new_tax=False):
        self._tax = tax or TaxCalculator()
        self._use_new_tax = use_new_tax
    def invoice(self, account, lines):
        tax = self._tax.compute(account, lines) if self._use_new_tax \
              else self._compute_tax(account, lines)   # old path still here
        return self._assemble(account, lines, tax)

# After (end state) — tax is its own tested unit; the God Object shrank by one responsibility.
class LegacyBilling:
    def __init__(self, tax: TaxCalculator):
        self._tax = tax
    def invoice(self, account, lines):
        tax = self._tax.compute(account, lines)
        return self._assemble(account, lines, tax)

Exercise 13 — The full combo cleanup¶

Anti-pattern: all five at once. Goal: refactor a single handler exhibiting God-function tendencies, Arrow nesting, a flag, a lava-flow branch, and a boat-anchor helper. Constraints: preserve the HTTP-style result; tackle the patterns in a safe order.

// Before — one function with a bit of everything.
func HandleUpload(req *Request, isAdmin bool) (int, string) {
    if req != nil {
        if req.User != nil {
            if req.User.Active {
                if len(req.Body) > 0 {
                    if len(req.Body) < maxSize || isAdmin {
                        // lava flow: old virus-scan stub, scanner removed in 2021
                        if false && scanLegacy(req.Body) {
                            return 500, "scan failed"
                        }
                        name := store(req.Body)
                        return 200, name
                    } else {
                        return 413, "too large"
                    }
                } else {
                    return 400, "empty"
                }
            } else {
                return 403, "inactive"
            }
        } else {
            return 401, "no user"
        }
    }
    return 400, "nil request"
}

// boat anchor: never called anywhere.
func validateChecksum(b []byte, want string) bool { /* 30 lines, zero call sites */ }

// After — flat, named, dead-code-free.
func HandleUpload(req *Request, isAdmin bool) (int, string) {
    if req == nil {
        return 400, "nil request"
    }
    if req.User == nil {
        return 401, "no user"
    }
    if !req.User.Active {
        return 403, "inactive"
    }
    if len(req.Body) == 0 {
        return 400, "empty"
    }
    if !withinSizeLimit(len(req.Body), isAdmin) {
        return 413, "too large"
    }
    return 200, store(req.Body)
}

func withinSizeLimit(n int, isAdmin bool) bool {
    return n < maxSize || isAdmin
}
// validateChecksum and scanLegacy: deleted (no callers / statically dead).

Refactoring discipline — the recap¶

The exercises above all run the same loop. Internalize it and the specific moves become mechanical:

green  →  one named refactoring  →  green  →  commit  →  repeat
          (behavioral change, if any, gets its OWN commit)

• Characterize before you change. A test that pins current behavior (bugs and all) is the seatbelt. Without it, "refactoring" is just editing and hoping. For production code you can't easily test, a parallel run (compute old and new, compare) is the characterization test (Exercise 12).
• Small, reversible steps. Extract one method, run tests, commit. If green turns red, your last step is the suspect — revert it. Small steps make git bisect and rollback trivial.
• Separate structural from behavioral commits. A refactor must preserve behavior; mixing in a bug fix destroys the signal that a red test gives you. When a fix is warranted (the 0.08 tax, the changed error strings), it gets its own reviewed commit after the structure is clean.
• Name the move. "Replace Nested Conditional with Guard Clauses," "Extract Class," "Replace Conditional with Polymorphism," "Introduce Parameter Object," "Combine Functions into Pipeline," "Inline Class," "Strangler Fig," "Branch by Abstraction." Naming the move means it's a known-safe, often IDE-automatable transformation — not an improvisation.
• Delete with evidence, not courage. Boat anchors fall to a call-site search; lava flow falls to coverage + telemetry + git blame. Git is the undo button, so deletion is cheap and reversible.
• Don't over-correct. Splitting a God Object into thirty pass-through layers just trades one anti-pattern for Lasagna Code. Aim for cohesive, not maximally small.

Related Topics¶

• tasks.md — guided exercises building these moves from scratch.
• find-bug.md — the spotting counterpart: identify the anti-pattern, don't fix it.
• junior.md · middle.md · senior.md — recognize → countermove → refactor-at-scale.
• Refactoring → Refactoring Techniques — the mechanical catalog for every move named above.
• Refactoring → Code Smells — Large Class, Long Method, Long Parameter List at the smell level.
• Clean Code → Classes — SRP and cohesion, the target state for Exercise 9.
• Clean Code → Functions — small functions, guard clauses, flag arguments.
• Over-Engineering — Lasagna Code, the over-correction to avoid when splitting a God Object.