Bad Shortcuts Anti-Patterns — Refactoring Practice¶

Category: Development Anti-Patterns → Bad Shortcuts — convenience taken now, paid back many times later. Covers (collectively): Copy-Paste Programming · Magic Numbers / Strings · Hard Coding · Cargo Cult Programming · Pokémon Exception Handling · Stringly-Typed Programming

These are not "spot the smell" puzzles — find-bug.md does that. Here the code is shortcut-ridden but already working, and your job is to transform it into clean code without changing behavior. The skill on display is the process, not just 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. (Replacing a swallowed exception is the one place where the point is to change behavior — there you pin the observable output, then deliberately widen what escapes.)
2. Take small, reversible steps. One named refactoring at a time (Extract Function, Replace Magic Literal with Symbolic Constant, Introduce Configuration, …), tests green after each, commit.
3. Separate structural from behavioral commits. A pure refactor preserves behavior; an error-strategy or config change is behavioral and gets its own reviewed commit.

Each worked solution names the canonical refactoring moves (Fowler's Refactoring, 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 — Name the magic literals¶

Anti-pattern: Magic Numbers / Strings. Goal: give every load-bearing literal a name that documents its meaning. Constraints: identical computed result; no behavior change; the constant's name must encode the concept, not the value.

// Before — what do 86400, 3, 0.08, and "3" mean?
boolean canRenew(Session s, User u) {
    if (s.ageSeconds() > 86400) {
        return false;
    }
    if (u.failedLogins() >= 3) {
        return false;
    }
    if (u.statusCode().equals("3")) {   // 3 = suspended
        return false;
    }
    return true;
}

double finalPrice(double base) {
    return base * 1.08;                 // 1.08 = ??
}

// After — each literal carries its meaning in its name.
static final long   MAX_SESSION_SECONDS  = 86_400;  // one day
static final int    MAX_FAILED_LOGINS    = 3;
static final String STATUS_SUSPENDED     = "3";     // TODO: promote to an enum (Ex. 7)
static final double SALES_TAX_RATE       = 0.08;

boolean canRenew(Session s, User u) {
    if (s.ageSeconds() > MAX_SESSION_SECONDS) return false;
    if (u.failedLogins() >= MAX_FAILED_LOGINS) return false;
    if (u.statusCode().equals(STATUS_SUSPENDED)) return false;
    return true;
}

double finalPrice(double base) {
    return base * (1 + SALES_TAX_RATE);
}

Exercise 2 — Extract the copy-pasted block¶

Anti-pattern: Copy-Paste Programming. Goal: collapse duplicated knowledge into one home. Constraints: preserve both return values exactly; the shared rule must have exactly one place to change.

# Before — the shipping rule is copied and will drift.
def price_for_member(items):
    total = sum(i.price for i in items)
    if total > 100:
        shipping = 0.0
    else:
        shipping = 9.99
    return round(total * 0.9 + shipping, 2)   # 10% member discount

def price_for_guest(items):
    total = sum(i.price for i in items)
    if total > 100:                            # SAME rule, copied
        shipping = 0.0
    else:
        shipping = 9.99
    return round(total + shipping, 2)

# After — the rule lives in exactly one function.
FREE_SHIPPING_THRESHOLD = 100
FLAT_SHIPPING_FEE       = 9.99
MEMBER_DISCOUNT         = 0.10

def shipping_for(total: float) -> float:
    return 0.0 if total > FREE_SHIPPING_THRESHOLD else FLAT_SHIPPING_FEE

def price(items, *, member: bool) -> float:
    total = sum(i.price for i in items)
    discount = MEMBER_DISCOUNT if member else 0.0
    return round(total * (1 - discount) + shipping_for(total), 2)

Exercise 3 — Leave the coincidental duplication alone¶

Anti-pattern: Copy-Paste — the false positive. Goal: recognize duplication that you must not merge, and make that decision explicit. Constraints: the two rules must remain independently changeable; no shared abstraction.

# Before — these LOOK identical. Should you DRY them?
def validate_username(s: str) -> bool:
    return 3 <= len(s) <= 20

def validate_coupon_code(s: str) -> bool:
    return 3 <= len(s) <= 20

# After — duplication kept on purpose; intent made explicit.
USERNAME_MIN, USERNAME_MAX = 3, 20   # human-identity rule
COUPON_MIN,   COUPON_MAX   = 3, 20   # marketing-format rule (independent of username)

def validate_username(s: str) -> bool:
    return USERNAME_MIN <= len(s) <= USERNAME_MAX

def validate_coupon_code(s: str) -> bool:
    return COUPON_MIN <= len(s) <= COUPON_MAX

Exercise 4 — Hard-coded values → configuration¶

Anti-pattern: Hard Coding. Goal: move environment-specific values out of source, treating secrets correctly. Constraints: the function signature stays usable; defaults must reproduce current behavior in dev; a committed secret must be rotated, not just moved.

// Before — credentials in source, a laptop-only path, an env-specific URL.
func newClient() *Client {
    return &Client{
        dsn:       "postgres://admin:S3cr3t!@10.0.0.5:5432/prod", // secret in git history!
        uploadDir: "/Users/alex/dev/app/uploads",                // only on Alex's machine
        apiBase:   "https://api.prod.example.com",               // env-specific
        timeout:   5 * time.Second,                              // arguably fine to keep
    }
}

// After — configuration loaded at the edge; secret never in source.
type Config struct {
    DSN       string // from secrets manager / injected env — NEVER a default in code
    UploadDir string
    APIBase   string
    Timeout   time.Duration
}

const defaultTimeout = 5 * time.Second // stable; not env-specific

func LoadConfig() (Config, error) {
    dsn := os.Getenv("DATABASE_URL")
    if dsn == "" {
        return Config{}, errors.New("DATABASE_URL is required") // fail fast; no fallback secret
    }
    return Config{
        DSN:       dsn,
        UploadDir: getenvOr("UPLOAD_DIR", "./uploads"),                 // safe dev default
        APIBase:   getenvOr("API_BASE", "https://api.dev.example.com"), // safe dev default
        Timeout:   defaultTimeout,
    }, nil
}

func newClient(cfg Config) *Client {
    return &Client{dsn: cfg.DSN, uploadDir: cfg.UploadDir, apiBase: cfg.APIBase, timeout: cfg.Timeout}
}

func getenvOr(key, fallback string) string {
    if v := os.Getenv(key); v != "" {
        return v
    }
    return fallback
}

Exercise 5 — Clean up the cargo-culted snippet¶

Anti-pattern: Cargo Cult Programming. Goal: remove lines kept "because the example had them," keeping only what is justified. Constraints: the function's output and the file it reads must be unchanged; every surviving line must be defensible.

# Before — pasted from a blog; nobody knows why half of this is here.
import os
import time
import pandas as pd

os.environ["TF_CPP_MIN_LOG_LEVEL"] = "3"   # copied; this code never imports TensorFlow

def load_active_users(path):
    time.sleep(0.1)                          # "the tutorial had a sleep"
    df = pd.read_csv(path)
    df = df.copy()                           # "saw this somewhere, seemed safe"
    df = df[df["active"] == True]            # the only line that does real work
    df = df.reset_index().reset_index()      # reset twice "just in case"
    return df

# After — every line earns its place.
import pandas as pd

def load_active_users(path):
    df = pd.read_csv(path)
    return df[df["active"]].reset_index(drop=True)

Exercise 6 — Catch the Pokémon exception¶

Anti-pattern: Pokémon Exception Handling. Goal: replace a catch-everything-and-swallow block with deliberate, typed handling that preserves the diagnostic trail. Constraints: expected failures stay handled with the same user-facing result; bugs must now propagate loudly instead of vanishing; the original cause must be preserved when wrapping.

// Before — "gotta catch 'em all", then silence.
ChargeResult charge(Order order) {
    try {
        int amount = Integer.parseInt(order.amountText());  // may be malformed
        gateway.charge(order.card(), amount);               // may decline, may time out
        return ChargeResult.ok();
    } catch (Exception e) {
        return ChargeResult.failed("could not charge");     // hides decline, timeout, AND bugs
    }
}

// After — each catch handles a real, distinct failure; bugs propagate.
ChargeResult charge(Order order) {
    int amount;
    try {
        amount = Integer.parseInt(order.amountText());
    } catch (NumberFormatException e) {
        return ChargeResult.failed("invalid amount: " + order.amountText());
    }

    try {
        gateway.charge(order.card(), amount);
        return ChargeResult.ok();
    } catch (CardDeclinedException e) {
        return ChargeResult.failed("card declined: " + e.reason());     // expected, recoverable
    } catch (GatewayTimeoutException e) {
        log.warn("gateway timeout for order {}", order.id(), e);         // logged WITH the cause
        throw new ChargeRetryableException("charge timed out", e);       // wrap + preserve cause
    }
    // No catch(Exception): a NullPointerException from a bug now crashes loudly and is found.
}

Exercise 7 — Stringly-typed status → enum¶

Anti-pattern: Stringly-Typed Programming. Goal: replace a String status (a fixed value set) with an enum so the compiler rejects typos and lists valid values. Constraints: the persisted/wire representation ("active", "suspended", "closed") stays byte-identical for backward compatibility; public method names unchanged.

// Before — String for a fixed set; "actve" compiles, fails in prod.
class Account {
    private String status;  // "active" | "suspended" | "closed" — by convention only

    void setStatus(String status) { this.status = status; }
    boolean canTransact() { return status.equals("active"); }
    boolean isSuspended() { return status.equals("suspended"); }
}

// elsewhere:
account.setStatus("suspeded");  // typo compiles cleanly; canTransact() silently wrong

// After — enum with stable wire values; illegal states unrepresentable.
enum Status {
    ACTIVE("active"), SUSPENDED("suspended"), CLOSED("closed");

    private final String wire;
    Status(String wire) { this.wire = wire; }
    String wireValue() { return wire; }

    static Status fromWire(String s) {
        for (Status v : values()) if (v.wire.equals(s)) return v;
        throw new IllegalArgumentException("unknown status: " + s);
    }
}

class Account {
    private Status status;

    void setStatus(Status status) { this.status = status; }   // setStatus("suspeded") no longer compiles
    boolean canTransact() { return status == Status.ACTIVE; }
    boolean isSuspended() { return status == Status.SUSPENDED; }
}

// boundary mapping (persistence/JSON):
account.setStatus(Status.fromWire(row.get("status")));   // read
row.put("status", account.status().wireValue());          // write

Exercise 8 — Distinct value types for confusable strings¶

Anti-pattern: Stringly-Typed Programming (confusable parameters). Goal: stop two string parameters that mean different things from being silently swapped, using distinct value types. Constraints: runtime representation stays a string; the bug class (transposed arguments) becomes a compile error.

// Before — both are strings; transposing them compiles and corrupts data.
func SendWelcome(userID string, email string) error {
    // ... uses userID for the audit log, email for delivery ...
    audit.Record(userID)
    return mailer.Send(email, "Welcome!")
}

// call site — arguments swapped; compiler says nothing:
SendWelcome("a@b.com", "u_42")   // emails "u_42", audits "a@b.com"

type UserID string
type Email  string

// After — distinct types; swapping is a compile error.
type UserID string
type Email  string

func NewEmail(s string) (Email, error) {
    if !strings.Contains(s, "@") {
        return "", fmt.Errorf("invalid email: %q", s)
    }
    return Email(s), nil
}

func SendWelcome(id UserID, to Email) error {
    audit.Record(string(id))
    return mailer.Send(string(to), "Welcome!")
}

// call site — transposition no longer compiles:
SendWelcome(UserID("u_42"), Email("a@b.com"))
// SendWelcome(Email("a@b.com"), UserID("u_42")) // ← compile error: types don't match

Exercise 9 — Magic strings → enum dispatch¶

Anti-pattern: Magic Strings + Stringly-Typed (string-based dispatch). Goal: replace an if/elif ladder over magic string literals with enum-keyed dispatch (or polymorphism). Constraints: identical computed results; an unknown type still raises the same error; behavior open for extension without editing the dispatcher.

# Before — magic strings drive a branching ladder; a typo is a runtime surprise.
def area(shape: str, **dims) -> float:
    if shape == "circle":
        return 3.14159 * dims["r"] ** 2
    elif shape == "square":
        return dims["side"] ** 2
    elif shape == "rect":
        return dims["w"] * dims["h"]
    else:
        raise ValueError(f"unknown shape: {shape}")

area("circel", r=2)   # typo → ValueError at runtime, far from the call

# After — enum-keyed table; the dispatcher never changes again.
import math
from enum import Enum

class ShapeKind(Enum):
    CIRCLE = "circle"
    SQUARE = "square"
    RECT   = "rect"

_AREA = {
    ShapeKind.CIRCLE: lambda d: math.pi * d["r"] ** 2,
    ShapeKind.SQUARE: lambda d: d["side"] ** 2,
    ShapeKind.RECT:   lambda d: d["w"] * d["h"],
}

def area(shape: ShapeKind, **dims) -> float:
    return _AREA[shape](dims)

# boundary parse from an external string keeps the old error shape:
def area_from_name(name: str, **dims) -> float:
    try:
        kind = ShapeKind(name)
    except ValueError:
        raise ValueError(f"unknown shape: {name}")
    return area(kind, **dims)

Exercise 10 — Copy-paste with a duplicated magic value¶

Anti-pattern: Copy-Paste + Magic Number (the two cluster). Goal: extract duplicated logic and the magic value it carries, so a rate change is a single edit. Constraints: same totals; the tax rate must live in exactly one place.

// Before — the tax rate 0.08 is copy-pasted into three methods.
class Invoice {
    double subtotalWithTax(List<Item> items) {
        double sub = 0;
        for (Item i : items) sub += i.price() * i.qty();
        return sub + sub * 0.08;                 // copy 1
    }
    double estimateTax(double amount) {
        return amount * 0.08;                    // copy 2
    }
    double grandTotal(double subtotal, double shipping) {
        return subtotal + shipping + subtotal * 0.08;  // copy 3
    }
}

// After — one rate, one tax computation, three callers.
class Invoice {
    private static final double SALES_TAX_RATE = 0.08;

    private static double tax(double base) {
        return base * SALES_TAX_RATE;
    }

    double subtotalWithTax(List<Item> items) {
        double sub = 0;
        for (Item i : items) sub += i.price() * i.qty();
        return sub + tax(sub);
    }
    double estimateTax(double amount) {
        return tax(amount);
    }
    double grandTotal(double subtotal, double shipping) {
        return subtotal + shipping + tax(subtotal);
    }
}

Exercise 11 — A deliberate typed-error strategy¶

Anti-pattern: Pokémon Exceptions + stringly-typed error signaling. Goal: turn a swallow-everything handler that signals failure via magic strings into a classified, typed error strategy that preserves causes and distinguishes recoverable failures from bugs. Constraints: the caller's observable outcomes for expected failures stay the same; bugs propagate; causes are chained.

# Before — bare except, then a stringly-typed "error code" return.
def process(body: str):
    try:
        data = json.loads(body)
        user = db.get(data["id"])
        charge(user, data["amount"])
        return "OK"
    except Exception:
        return "ERR"        # which error? bad JSON? missing user? a bug? nobody can tell

# After — classified, typed, cause-preserving.
from dataclasses import dataclass

@dataclass
class Ok: ...
@dataclass
class Rejected:           # expected, recoverable — carries a reason
    reason: str

def process(body: str) -> Ok | Rejected:
    try:
        data = json.loads(body)
    except json.JSONDecodeError as e:
        raise InvalidRequest("malformed JSON") from e   # expected; cause preserved

    if "id" not in data or "amount" not in data:
        return Rejected("missing id or amount")          # domain condition, handled explicitly

    user = db.get(data["id"])
    if user is None:
        return Rejected("unknown user")                  # domain condition

    try:
        charge(user, data["amount"])
    except PaymentDeclined as e:
        return Rejected(f"declined: {e.reason}")         # expected, recoverable
    # any other exception (a bug) propagates to the boundary and is logged with a trace
    return Ok()

Exercise 12 — Hard-coded business rules → config + value type¶

Anti-pattern: Hard Coding + Magic Numbers + Stringly-Typed (combined). Goal: lift environment-varying business rules out of source, name the magic, and replace a stringly-typed tier with a value type. Constraints: dev defaults reproduce current behavior; backward-compatible config keys; the tier set is closed.

// Before — magic thresholds, a stringly-typed tier, and an env-specific limit baked in.
func DiscountFor(tier string, cartTotal float64) float64 {
    if tier == "gold" {
        if cartTotal > 500 {        // 500 = ?? and differs by region/promo
            return cartTotal * 0.15 // 0.15 = ??
        }
        return cartTotal * 0.10
    } else if tier == "silver" {
        return cartTotal * 0.05
    }
    return 0 // "bronze" or anything unrecognized
}

// After — named rules, configurable threshold, closed tier set.
type Tier int

const (
    Bronze Tier = iota
    Silver
    Gold
)

func TierFromString(s string) (Tier, bool) {
    switch s {
    case "gold":
        return Gold, true
    case "silver":
        return Silver, true
    case "bronze":
        return Bronze, true
    }
    return Bronze, false // unknown → Bronze (0% discount), preserving old behavior
}

const (
    goldHighRate = 0.15
    goldBaseRate = 0.10
    silverRate   = 0.05
)

type DiscountConfig struct {
    BulkThreshold float64 // per-environment; default 500
}

func LoadDiscountConfig() DiscountConfig {
    return DiscountConfig{BulkThreshold: getenvFloat("BULK_THRESHOLD", 500)}
}

func DiscountFor(tier Tier, cartTotal float64, cfg DiscountConfig) float64 {
    switch tier {
    case Gold:
        if cartTotal > cfg.BulkThreshold {
            return cartTotal * goldHighRate
        }
        return cartTotal * goldBaseRate
    case Silver:
        return cartTotal * silverRate
    default:
        return 0
    }
}

Exercise 13 — The full shortcut cleanup¶

Anti-pattern: all six at once. Goal: refactor one function exhibiting copy-paste, magic literals, a hard-coded URL, a cargo-culted line, a Pokémon catch, and a stringly-typed mode. Constraints: preserve the observable result for expected inputs; tackle the shortcuts in a safe order; secrets/URLs out of source.

# Before — a bit of every shortcut.
import os, time, requests

def fetch_report(mode, account_id):
    time.sleep(0.2)                                    # cargo cult: copied from a retry tutorial
    try:
        if mode == "json":                             # stringly-typed mode + magic strings
            url = "https://api.prod.example.com/v1/report"   # hard-coded, env-specific
            r = requests.get(url, params={"id": account_id}, timeout=30)  # magic 30
            data = r.json()
            return {"id": data["id"], "score": data["score"] * 1.1}       # magic 1.1
        elif mode == "csv":
            url = "https://api.prod.example.com/v1/report"   # SAME url copy-pasted
            r = requests.get(url, params={"id": account_id, "fmt": "csv"}, timeout=30)
            return r.text
        else:
            return None
    except Exception:                                  # Pokémon: swallows everything
        return None

# After — every shortcut addressed; behavior preserved for expected inputs.
import os
from enum import Enum
import requests

REQUEST_TIMEOUT  = 30          # seconds
SCORE_MULTIPLIER = 1.1

class ReportFormat(Enum):
    JSON = "json"
    CSV  = "csv"

def _api_base() -> str:
    return os.getenv("API_BASE", "https://api.dev.example.com")  # configurable; safe dev default

def _get_report(account_id, *, params=None):
    url = f"{_api_base()}/v1/report"
    return requests.get(url, params={"id": account_id, **(params or {})}, timeout=REQUEST_TIMEOUT)

def fetch_report(fmt: ReportFormat, account_id):
    try:
        if fmt is ReportFormat.JSON:
            data = _get_report(account_id).json()
            return {"id": data["id"], "score": data["score"] * SCORE_MULTIPLIER}
        # fmt is ReportFormat.CSV (enum makes this exhaustive):
        return _get_report(account_id, params={"fmt": "csv"}).text
    except requests.Timeout as e:
        raise ReportUnavailable("report request timed out") from e   # recoverable; cause kept
    except requests.HTTPError as e:
        raise ReportUnavailable("upstream error") from e             # cause kept
    # a KeyError (missing "score") or any bug now propagates and gets fixed

def fetch_report_by_name(mode: str, account_id):
    try:
        fmt = ReportFormat(mode)
    except ValueError:
        return None                                                  # unknown mode → None (preserved)
    return fetch_report(fmt, account_id)

Refactoring discipline — the recap¶

Every exercise above runs the same loop. Internalize it and the specific moves become mechanical:

green  →  one named refactoring  →  green  →  commit  →  repeat
          (behavioral change — error strategy, config, precision fix — gets its OWN commit)

• Characterize before you change. A test that pins current behavior (bugs and all) is the seatbelt. For the Pokémon-exception exercises the point is to change behavior — there you pin the observable expected-failure outcomes first, then deliberately widen what escapes, and label the commit behavioral.
• Small, reversible steps. Name the constant, run tests, commit. Extract the function, run tests, commit. If green turns red, your last step is the suspect — revert it.
• Separate structural from behavioral commits. Naming a magic value or extracting a duplicate is structural and must preserve output byte-for-byte (watch algebraic rewrites like base*1.08 → base*(1+0.08) and 3.14159 → math.pi). Changing an error strategy, adding validation, or introducing config is behavioral — its own reviewed commit.
• Name the move. "Replace Magic Literal with Symbolic Constant," "Extract Function," "Introduce Configuration," "Replace Type Code with Enum," "Introduce Value Type," "Replace String Dispatch with Enum/Polymorphism," "Replace Bare Catch with Typed Handling." A named move is a known-safe, often IDE-automatable transformation — not an improvisation.
• Restraint is a refactoring too. Exercise 3's correct answer was not to merge. DRY couples what it touches; only merge knowledge that must change together.
• Secrets are special and irreversible. Moving a credential out of source does not un-leak it — rotate it and add a secret scanner. This is the one shortcut whose cost is catastrophic.

Related Topics¶

• tasks.md — guided exercises building these moves from scratch.
• find-bug.md — the spotting counterpart: identify the shortcut, 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 — Duplicate Code, Magic Number, Primitive Obsession at the smell level.
• Clean Code → Error Handling — the target state for the Pokémon-exception exercises.
• Clean Code → Meaningful Names — naming away magic values.
• DRY Principle — and its limit, the coincidental-duplication trap (Exercise 3).
• Secrets Management — handling and rotating credentials (Exercise 4).
• Over-Engineering — Soft Coding, the over-correction to avoid when introducing configuration.
• Bad Structure — the sibling refactoring file; same discipline, different anti-patterns.