Bad Structure Anti-Patterns — Find the Bug¶

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

This file is critical-reading practice. Each snippet below is a plausible chunk of real-world code in Go, Java, or Python. Your job is to read it the way a good reviewer does and answer three questions:

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

These are structural anti-patterns, so the "bug" is usually not a crash on line one — it's a latent flaw in the shape of the code that makes it expensive to change. But structure isn't only aesthetic: bad shape hides and causes real functional bugs, and several snippets here contain one. Some snippets are deliberately innocent-looking — code that would sail through a casual review. Read slowly, then open the answer.

How to use this file: read each snippet and write your own answer before expanding the collapsible. The skill you're training is noticing the shape, not recalling the name.

Table of Contents¶

1. The dispatcher that grew a tail
2. A reasonable-looking order service
3. Three steps and a dictionary
4. The validator pyramid
5. The exporter nobody calls
6. The comment that guards the code
7. A cache with a helpful method
8. The discount calculator
9. The interface with one implementation
10. The retry loop that learned to flag
11. The report builder
12. Two ways to compute the same total
13. The settings singleton
14. The nested permission check

Snippet 1 — The dispatcher that grew a tail¶

# Python — an event handler in a long-lived service
def handle_event(event, ctx):
    if event["type"] == "order.created":
        if event["payload"].get("items"):
            if ctx.user.is_active:
                order = build_order(event["payload"])
                if order.total > 0:
                    if charge(ctx.user, order.total):
                        save(order)
                        send_email(ctx.user, "order_confirmed", order)
                        return {"ok": True}
                    else:
                        return {"ok": False, "reason": "charge_failed"}
                else:
                    return {"ok": False, "reason": "empty_total"}
            else:
                return {"ok": False, "reason": "inactive_user"}
        else:
            return {"ok": False, "reason": "no_items"}
    elif event["type"] == "order.cancelled":
        # ... another 30 lines, same shape ...
        pass

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

HANDLERS = {"order.created": _on_created, "order.cancelled": _on_cancelled}

def handle_event(event, ctx):
    handler = HANDLERS.get(event["type"])
    if handler is None:
        return {"ok": False, "reason": "unknown_event"}
    return handler(event["payload"], ctx)

def _on_created(payload, ctx):
    if not payload.get("items"):   return {"ok": False, "reason": "no_items"}
    if not ctx.user.is_active:     return {"ok": False, "reason": "inactive_user"}
    order = build_order(payload)
    if order.total <= 0:           return {"ok": False, "reason": "empty_total"}
    if not charge(ctx.user, order.total):
        return {"ok": False, "reason": "charge_failed"}
    save(order)
    send_email(ctx.user, "order_confirmed", order)
    return {"ok": True}

Snippet 2 — A reasonable-looking order service¶

// Java — looks like a normal service class; would pass a casual review
public class OrderService {
    private final Database db;
    private final SmtpClient smtp;
    private final PaymentGateway gateway;
    private final PdfRenderer pdf;
    private final S3Client s3;
    private final SlackClient slack;
    private final InventoryApi inventory;
    private final TaxApi tax;

    public Order create(OrderRequest r) { /* validate, price, persist */ }
    public void charge(Order o)         { /* gateway.charge(...) */ }
    public void refund(Order o)         { /* gateway.refund(...) */ }
    public byte[] invoicePdf(Order o)   { /* pdf.render(...) */ }
    public void emailInvoice(Order o)   { /* smtp.send(...) */ }
    public void archiveInvoice(Order o) { /* s3.put(...) */ }
    public void notifyOps(Order o)      { /* slack.post(...) */ }
    public boolean inStock(Order o)     { /* inventory.check(...) */ }
    public BigDecimal taxFor(Order o)   { /* tax.compute(...) */ }
    public void exportDailyCsv()        { /* db + file I/O */ }
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

class OrderCreation { OrderCreation(Database db, PricingService p) {...} }
class Billing       { Billing(PaymentGateway g) {...} }   // charge, refund
class Invoicing     { Invoicing(PdfRenderer pdf, SmtpClient s, S3Client s3) {...} }
class OpsNotifier   { OpsNotifier(SlackClient slack) {...} }
// OrderService (if it survives) just wires these together for one use case.

Snippet 3 — Three steps and a dictionary¶

# Python — an import job split across three functions
_ctx = {}

def load(path):
    _ctx["rows"] = read_csv(path)
    _ctx["loaded"] = True

def transform():
    rows = _ctx["rows"]                      # KeyError if load() didn't run
    _ctx["clean"] = [normalize(r) for r in rows]

def store():
    if _ctx.get("loaded"):
        db.bulk_insert(_ctx["clean"])        # KeyError if transform() didn't run
        _ctx.clear()

def run_import(path):
    load(path)
    store()                                  # <-- note the call order
    transform()

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

def run_import(path):
    rows  = read_csv(path)
    clean = [normalize(r) for r in rows]
    db.bulk_insert(clean)

Snippet 4 — The validator pyramid¶

// Go — request validation in an HTTP handler
func validate(req *Request) (*Account, error) {
    if req != nil {
        if req.Token != "" {
            acct, err := lookup(req.Token)
            if err == nil {
                if acct != nil {
                    if acct.Verified {
                        if req.Amount > 0 {
                            if req.Amount <= acct.Balance {
                                return acct, nil
                            } else {
                                return nil, errors.New("insufficient funds")
                            }
                        } else {
                            return nil, errors.New("amount must be positive")
                        }
                    } else {
                        return nil, errors.New("account not verified")
                    }
                }
            }
        }
    }
    return nil, errors.New("invalid request")
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

func validate(req *Request) (*Account, error) {
    if req == nil || req.Token == "" {
        return nil, errors.New("invalid request")
    }
    acct, err := lookup(req.Token)
    if err != nil {
        return nil, fmt.Errorf("lookup failed: %w", err)   // no longer swallowed
    }
    if acct == nil {
        return nil, errors.New("account not found")
    }
    if !acct.Verified {
        return nil, errors.New("account not verified")
    }
    if req.Amount <= 0 {
        return nil, errors.New("amount must be positive")
    }
    if req.Amount > acct.Balance {
        return nil, errors.New("insufficient funds")
    }
    return acct, nil
}

Snippet 5 — The exporter nobody calls¶

// Go — an export package in a 4-year-old codebase
type Exporter interface {
    CSV(r Report) ([]byte, error)
    JSON(r Report) ([]byte, error)
    XML(r Report) ([]byte, error)
    XLSX(r Report) ([]byte, error)
    Parquet(r Report) ([]byte, error)
}

type exporter struct{ /* ... */ }

func (e *exporter) CSV(r Report) ([]byte, error)     { /* used everywhere */ }
func (e *exporter) JSON(r Report) ([]byte, error)    { /* used by one endpoint */ }
func (e *exporter) XML(r Report) ([]byte, error)     { /* 120 lines; zero call sites */ }
func (e *exporter) XLSX(r Report) ([]byte, error)    { /* 200 lines; zero call sites */ }
func (e *exporter) Parquet(r Report) ([]byte, error) { /* 90 lines; zero call sites */ }

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

type Exporter interface {
    CSV(r Report) ([]byte, error)
    JSON(r Report) ([]byte, error)
}

Snippet 6 — The comment that guards the code¶

# Python — pricing logic in a checkout module
def final_price(cart, user):
    subtotal = sum(i.price * i.qty for i in cart.items)

    # ---- DO NOT REMOVE ----
    # Legacy promo adjustment. Breaks the EU tax report if deleted (2021).
    # Nobody is sure why. Ask Dmitri (left the company). Keep it.
    legacy = 0
    if user.country in ("DE", "FR", "IT"):
        legacy = round(subtotal * 0.0)        # multiplies by zero
        subtotal = subtotal - legacy
    # ------------------------

    discount = apply_coupons(cart, user)
    return subtotal - discount

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

def final_price(cart, user):
    subtotal = sum(i.price * i.qty for i in cart.items)
    discount = apply_coupons(cart, user)
    return subtotal - discount

Snippet 7 — A cache with a helpful method¶

// Java — an in-memory cache shared across request threads
public class UserCache {
    private final Map<Long, User> map = new HashMap<>();

    public User get(long id) {
        User u = map.get(id);
        if (u == null) {
            u = db.load(id);          // expensive
            map.put(id, u);
        }
        return u;
    }

    // "Convenience" method added later by another team
    public int getAndIncrementVisits(long id) {
        User u = get(id);
        u.visits = u.visits + 1;      // mutates the shared cached object
        return u.visits;
    }
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

public class UserCache {                       // one responsibility: caching
    private final ConcurrentMap<Long, User> map = new ConcurrentHashMap<>();
    public User get(long id) { return map.computeIfAbsent(id, db::load); }
}

public class VisitCounter {                    // separate, thread-safe concern
    private final ConcurrentMap<Long, AtomicInteger> visits = new ConcurrentHashMap<>();
    public int increment(long id) {
        return visits.computeIfAbsent(id, k -> new AtomicInteger()).incrementAndGet();
    }
}

Snippet 8 — The discount calculator¶

# Python — discount engine reused across many call sites over the years
def calc_discount(price, customer, *, is_holiday=False, is_clearance=False,
                  is_employee=False, stack=False, legacy_mode=False):
    d = 0
    if legacy_mode:
        d = price * 0.05
    else:
        if is_employee:
            d = price * 0.30
            if stack and is_holiday:
                d = d + price * 0.10
        elif is_holiday:
            d = price * 0.15
            if is_clearance:
                if stack:
                    d = d + price * 0.20
                else:
                    d = price * 0.20      # replaces, not adds
        elif is_clearance:
            d = price * 0.25
    if customer.tier == "gold" and not legacy_mode:
        d = d + price * 0.05
    return min(d, price)

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

def calc_discount(price, customer):
    rules = active_discount_rules(customer)        # returns list of Rule objects
    total = sum(r.amount(price) for r in rules if r.applies(customer))
    return min(total, price)

Snippet 9 — The interface with one implementation¶

// Go — a "clean architecture" repository layer
type UserRepository interface {
    FindByID(id int64) (*User, error)
    FindByEmail(email string) (*User, error)
    Save(u *User) error
    Delete(id int64) error
}

type postgresUserRepository struct{ db *sql.DB }   // the only implementation

func (r *postgresUserRepository) FindByID(id int64) (*User, error) { /* ... */ }
func (r *postgresUserRepository) FindByEmail(e string) (*User, error) { /* ... */ }
func (r *postgresUserRepository) Save(u *User) error { /* ... */ }
func (r *postgresUserRepository) Delete(id int64) error { /* ... */ }

// In tests:
func TestSignup(t *testing.T) {
    repo := &mockUserRepository{}   // a second implementation, used by tests
    ...
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

Snippet 10 — The retry loop that learned to flag¶

// Java — an HTTP client wrapper, evolved over several tickets
public Response call(Request req, boolean retry, boolean cache,
                     boolean async, boolean logBody) {
    if (cache) {
        Response cached = CACHE.get(req.key());
        if (cached != null) {
            if (logBody) { LOG.info(cached.body()); }
            return cached;
        }
    }
    Response resp = null;
    int attempts = retry ? 3 : 1;
    for (int i = 0; i < attempts; i++) {
        try {
            resp = transport.send(req, async);
            if (resp.code() < 500) { break; }
        } catch (IOException e) {
            if (i == attempts - 1) { throw new RuntimeException(e); }
        }
    }
    if (cache && resp != null) { CACHE.put(req.key(), resp); }
    if (logBody && resp != null) { LOG.info(resp.body()); }
    return resp;
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

Response resp = retryPolicy.execute(() -> transport.send(req));   // retry isolated
// caching + logging applied by wrapping clients, not by booleans inside call()

Snippet 11 — The report builder¶

# Python — generates a weekly metrics report; called by a cron job
class ReportBuilder:
    def __init__(self, db, mailer, s3, pdf, slack, config, clock, cache):
        self.db, self.mailer, self.s3 = db, mailer, s3
        self.pdf, self.slack, self.config = pdf, slack, config
        self.clock, self.cache = clock, cache

    def build_and_send(self, team_id):
        data = self._query(team_id)
        if data:
            if self.config.get("pdf_enabled"):
                pdf_bytes = self._render_pdf(data)
                if self.config.get("archive_enabled"):
                    url = self._archive(pdf_bytes)
                    if self.config.get("notify_enabled"):
                        if self.config.get("use_slack"):
                            self._slack(team_id, url)
                        else:
                            self._email(team_id, pdf_bytes)
        # old XML path — superseded by PDF in 2022, kept "in case finance asks"
        # xml = self._render_xml(data); self.s3.put(...)   # commented out

    def _render_xml(self, data): ...   # 80 lines, only caller is commented out

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

def build_and_send(self, team_id):
    data = self.metrics.query(team_id)
    if not data:                       return
    if not self.config.pdf_enabled:    return
    pdf = self.report.render(data)
    if self.config.archive_enabled:    self.archiver.store(pdf)
    self.notifier.send(team_id, pdf)   # no longer trapped inside archiving

Snippet 12 — Two ways to compute the same total¶

// Java — an AccountManager that has accumulated everything about accounts
public class AccountManager {
    private List<Transaction> txns = new ArrayList<>();
    private BigDecimal cachedBalance = BigDecimal.ZERO;

    public void addTransaction(Transaction t) {
        txns.add(t);
        cachedBalance = cachedBalance.add(t.amount());   // keep cache in sync
    }

    public BigDecimal getBalance() {
        return cachedBalance;                            // fast path
    }

    public BigDecimal getStatementBalance() {            // used by the PDF statement
        BigDecimal sum = BigDecimal.ZERO;
        for (Transaction t : txns) {
            if (!t.isPending()) {                        // statements exclude pending
                sum = sum.add(t.amount());
            }
        }
        return sum;
    }

    public void removeTransaction(Transaction t) {
        txns.remove(t);                                  // forgot to update cachedBalance!
    }
    // ... 30 more methods: PDF, email, CSV export, audit log ...
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

class Ledger {                                  // one responsibility: the truth about money
    private final List<Transaction> txns = new ArrayList<>();
    void add(Transaction t)    { txns.add(t); }
    void remove(Transaction t) { txns.remove(t); }
    BigDecimal balance()           { return sum(txns, t -> true); }
    BigDecimal statementBalance()  { return sum(txns, t -> !t.isPending()); }
    // no cached field to fall out of sync; cache later only if profiling demands it
}

Snippet 13 — The settings singleton¶

# Python — global settings used throughout the app
class Settings:
    _instance = None

    def __new__(cls):
        if cls._instance is None:
            cls._instance = super().__new__(cls)
        return cls._instance

    def __init__(self):
        self.db_url = os.environ["DB_URL"]
        self.timeout = 30
        self.feature_x = False

settings = Settings()

# elsewhere, in a request handler:
def handle(req):
    if req.beta_user:
        settings.feature_x = True        # flip the global flag for beta users
    result = run_pipeline(settings)
    return result

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

@dataclass(frozen=True)
class Settings:                       # immutable, read-only
    db_url: str
    timeout: int = 30

def handle(req, settings):            # settings injected, not global
    ctx = RequestContext(feature_x=req.beta_user)   # per-request, not shared
    return run_pipeline(settings, ctx)

Snippet 14 — The nested permission check¶

// Go — middleware-style authorization, would pass a quick review
func authorize(u *User, res *Resource, action string) bool {
    if u != nil {
        if !u.Banned {
            if u.Role == "admin" {
                return true
            } else {
                if res.OwnerID == u.ID {
                    if action == "read" || action == "write" {
                        return true
                    } else if action == "delete" {
                        if u.Role == "editor" {
                            return true
                        }
                    }
                } else {
                    if res.IsPublic {
                        if action == "read" {
                            return true
                        }
                    }
                }
            }
        }
    }
    return false
}

What anti-pattern(s) are present? What concrete problem(s) does it cause? How would you fix it?

func authorize(u *User, res *Resource, action string) bool {
    if u == nil || u.Banned { return false }   // deny by default, fail closed
    if u.Role == "admin"    { return true }

    if res.OwnerID == u.ID {
        switch action {
        case "read", "write": return true
        case "delete":        return u.Role == "editor"
        }
        return false
    }
    if res.IsPublic && action == "read" { return true }
    return false
}

Summary — patterns of spotting¶

You don't spot structural anti-patterns by recognizing a single bad line — you spot them by shape and by the questions you ask. The repeatable moves from these fourteen snippets:

• Count the constructor arguments / injected collaborators. Eight unrelated dependencies is a God Object regardless of how short each method is (Snippets 2, 11, 12). It's the fastest God-Object detector and survives casual review.
• Count the boolean parameters. More than ~2 flags braiding orthogonal concerns is Spaghetti forming, and the combinatorial paths reliably hide a bug (Snippets 8, 10).
• Find the deepest indent and read the fall-through. In Arrow code the real logic is deepest and the default return at the bottom is where swallowed errors and wrong decisions hide (Snippets 1, 4, 14). Security/money code nested this deep is a bug magnet.
• Trace shared mutable state across functions/threads/requests. Order-dependence (Snippet 3), concurrent mutation (Snippets 7, 13), and a forgotten cache update (Snippet 12) are bugs the coupling causes — each function "looks fine" alone.
• Interrogate dead and unused code. "DO NOT REMOVE, nobody's sure why" is Lava Flow (Snippet 6); a fully-built feature with zero call sites is a Boat Anchor (Snippets 5, 11). Both are deletable — git is the safety net.
• Resist false positives. An interface with one production impl is justified if tests (or a real contract) consume it — Boat Anchors are defined by purpose, not shape (Snippet 9). Count the real consumers, including tests, before crying "over-abstraction."

The meta-lesson: bad structure isn't only aesthetic. A God Object caused a lost-update race; an Arrow swallowed a 500 as a 400; flag-soup hid a discount-overwrite; a mutable singleton leaked a beta flag to every user. When the shape fights you, look hard — the latent functional bug is often hiding in exactly the place the structure made hard to read.

Related Topics¶

• junior.md — what each of the five shapes looks like and why it's bad.
• middle.md — when these creep in and the small countermove that reverses them.
• tasks.md — exercises that build the same muscles from the writing side.
• optimize.md — flawed implementations to refactor end-to-end.
• interview.md — Q&A across all levels.
• Clean Code → Classes — the SRP cure for God Objects.
• Clean Code → Functions — guard clauses and flag-argument removal.
• Refactoring → Code Smells — Large Class, Long Method, the smell-level view.
• Bad Shortcuts and Over-Engineering — the sibling find-bug files.