Functions — Find the Bug¶

12 snippets where a flaw in function design — a flag argument, an output parameter, hidden temporal coupling, a command-query mix, a returned null, a hidden side effect — causes or hides a real defect. Find the bug first; then notice that the shape of the function is what let the bug in. Fixing the shape usually makes the bug impossible to reintroduce.

The lesson of this chapter: most function bugs are not arithmetic mistakes. They are interface mistakes. A function whose signature lies about what it does, what it touches, or what order it must be called in will eventually be called incorrectly — by you, in six months, in good faith.

Table of Contents¶

• How to Use
• The shape of a function bug
• Snippet 1 — Flag argument takes the wrong branch (Go)
• Snippet 2 — Output parameter aliasing (Python)
• Snippet 3 — Hidden temporal coupling → NPE (Java)
• Snippet 4 — Command-query mix corrupts state on retry (Go)
• Snippet 5 — Returning null leads to NPE (Java)
• Snippet 6 — A "pure" helper with a hidden side effect (Python)
• Snippet 7 — One function doing two things; one half fails silently (Go)
• Snippet 8 — Default mutable argument (Python)
• Snippet 9 — Boolean flag inverts on a refactor (Java)
• Snippet 10 — Mixed abstraction levels hide a skipped guard (Go)
• Snippet 11 — Query method that mutates a cache, called under a read lock (Java)
• Snippet 12 — Output parameter "filled in" only on the happy path (Python)
• Scorecard
• Related Topics

How to Use¶

For each snippet:

1. Read the code and the calling site. Assume the caller is reasonable — the bug is almost never "the caller is an idiot." It is "the signature invited this mistake."
2. Answer the prompt before opening the <details>. Say out loud: what does this function promise, and where does it break that promise?
3. Open the answer. It names the bug, then the root cause in function design (flag / output param / temporal coupling / CQS violation / null / hidden effect), then the fix that removes the entire class of bug.

Score yourself with the Scorecard. A senior reviewer should catch 10+ of these on a first read.

The shape of a function bug¶

Every fix below pushes the same direction: make the function's interface honest, so the compiler, the reader, or the type system catches the next person before they ship the bug.

Snippet 1 — Flag argument takes the wrong branch (Go)¶

Difficulty: Easy

// SendReport renders a report and delivers it.
// If draft is true, it is saved internally and NOT emailed.
func SendReport(r Report, recipients []string, draft bool) error {
    body := render(r)
    if draft {
        return archive(body) // save only
    }
    return email(recipients, body)
}

// Caller A — nightly job that emails finished reports:
func runNightly(reports []Report) error {
    for _, r := range reports {
        // We want these EMAILED. They are final.
        if err := SendReport(r, mailingList, r.IsDraft()); err != nil {
            return err
        }
    }
    return nil
}

What's wrong? The nightly job is supposed to email final reports. Some of them go out, some don't. Why?

func EmailReport(r Report, recipients []string) error {
    return email(recipients, render(r))
}

func ArchiveDraft(r Report) error {
    return archive(render(r))
}

// Caller A is now unambiguous and cannot "accidentally" archive:
func runNightly(reports []Report) error {
    for _, r := range reports {
        if err := EmailReport(r, mailingList); err != nil {
            return err
        }
    }
    return nil
}

Snippet 2 — Output parameter aliasing (Python)¶

Difficulty: Medium

def apply_defaults(config: dict, defaults: dict) -> dict:
    """Return config with any missing keys filled from defaults."""
    for key, value in defaults.items():
        config.setdefault(key, value)
    return config


BASE_DEFAULTS = {"timeout": 30, "retries": 3, "region": "us-east-1"}


def build_client(user_config: dict):
    cfg = apply_defaults(user_config, BASE_DEFAULTS)
    return Client(cfg)


# Somewhere else, two requests in the same process:
build_client({"region": "eu-west-1"})   # request 1
build_client({})                          # request 2: expects us-east-1

What's wrong? Request 2 passes an empty config and expects the defaults. Run them in this order and request 2 is correct. Now reverse the order — or run request 1 a few times. Things drift. Why?

def apply_defaults(config: dict, defaults: dict) -> dict:
    """Return a NEW dict: defaults overlaid with config (config wins)."""
    return {**defaults, **config}

Snippet 3 — Hidden temporal coupling → NPE (Java)¶

Difficulty: Medium

public class ReportExporter {
    private List<Row> rows;
    private Formatter formatter;

    public void load(DataSource source) {
        this.rows = source.fetchRows();
    }

    public void configure(ExportOptions options) {
        this.formatter = new Formatter(options);
    }

    public byte[] export() {
        StringBuilder sb = new StringBuilder();
        for (Row row : rows) {                 // line X
            sb.append(formatter.format(row));   // line Y
        }
        return sb.toString().getBytes(UTF_8);
    }
}

// Caller, new endpoint added under deadline:
ReportExporter exporter = new ReportExporter();
exporter.configure(options);
byte[] out = exporter.export();   // <-- ships to prod

What's wrong? It compiles. It passes the one unit test (which happened to call load first). In production it throws. Where, and why?

public final class ReportExporter {
    // No mutable setup fields. Everything export needs is passed in.
    public byte[] export(List<Row> rows, ExportOptions options) {
        Objects.requireNonNull(rows, "rows");
        Formatter formatter = new Formatter(options);
        StringBuilder sb = new StringBuilder();
        for (Row row : rows) {
            sb.append(formatter.format(row));
        }
        return sb.toString().getBytes(UTF_8);
    }
}

// Caller cannot forget to provide the data — the compiler enforces it:
byte[] out = new ReportExporter().export(source.fetchRows(), options);

Snippet 4 — Command-query mix corrupts state on retry (Go)¶

Difficulty: Hard

type Counter struct {
    mu    sync.Mutex
    value int
}

// NextID returns the next ID. (Looks like a query: it "returns the next ID".)
func (c *Counter) NextID() int {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.value++
    return c.value
}

// Caller: build an audit record. Logged at two levels for debugging.
func recordEvent(c *Counter, e Event) AuditRecord {
    log.Printf("allocating id for event %q (will be %d)", e.Name, c.NextID())
    rec := AuditRecord{
        ID:    c.NextID(),
        Event: e,
    }
    log.Printf("recorded event %q as id %d", e.Name, rec.ID)
    return rec
}

What's wrong? IDs are skipping — the audit table has gaps (1, then 3, then 5...), and the "will be N" log never matches the actual ID. Why?

// Name says it mutates. CQS is respected: this is a command that returns
// the allocated value, and callers must treat each call as consuming one ID.
func (c *Counter) Allocate() int {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.value++
    return c.value
}

func recordEvent(c *Counter, e Event) AuditRecord {
    id := c.Allocate() // called exactly once
    log.Printf("allocating id %d for event %q", id, e.Name)
    rec := AuditRecord{ID: id, Event: e}
    log.Printf("recorded event %q as id %d", e.Name, rec.ID)
    return rec
}

Snippet 5 — Returning null leads to NPE (Java)¶

Difficulty: Easy

public class UserRepository {
    public User findByEmail(String email) {
        for (User u : allUsers) {
            if (u.getEmail().equalsIgnoreCase(email)) {
                return u;
            }
        }
        return null; // not found
    }
}

// Caller:
public boolean isAdmin(String email) {
    User user = userRepository.findByEmail(email);
    return user.getRoles().contains(Role.ADMIN);
}

What's wrong? Admin checks work for everyone in the database. Then someone passes an email that isn't registered, and the request 500s. Why?

public Optional<User> findByEmail(String email) {
    return allUsers.stream()
        .filter(u -> u.getEmail().equalsIgnoreCase(email))
        .findFirst();
}

// Caller is forced to handle "not found":
public boolean isAdmin(String email) {
    return userRepository.findByEmail(email)
        .map(u -> u.getRoles().contains(Role.ADMIN))
        .orElse(false);
}

Snippet 6 — A "pure" helper with a hidden side effect (Python)¶

Difficulty: Medium

def normalize_scores(scores: list[float]) -> list[float]:
    """Return scores scaled to 0..1. Pure helper, safe to call anywhere."""
    hi = max(scores)
    lo = min(scores)
    span = hi - lo or 1.0
    for i in range(len(scores)):
        scores[i] = (scores[i] - lo) / span   # <-- (!)
    return scores


def report(raw: list[float]):
    normalized = normalize_scores(raw)
    print("normalized:", normalized)
    print("raw average:", sum(raw) / len(raw))   # expects ORIGINAL raw

What's wrong? "raw average" prints a number between 0 and 1, not the average of the original scores. The docstring says the helper is pure. Where did the raw data go?

def normalize_scores(scores: list[float]) -> list[float]:
    """Return a NEW list of scores scaled to 0..1. Does not modify input."""
    hi, lo = max(scores), min(scores)
    span = hi - lo or 1.0
    return [(s - lo) / span for s in scores]

Snippet 7 — One function doing two things; one half fails silently (Go)¶

Difficulty: Medium

// SaveAndNotify persists the order and emails the customer.
func SaveAndNotify(o Order) error {
    if err := db.Save(o); err != nil {
        return err
    }
    // best-effort notify; we don't want email problems to fail the save
    _ = notifier.Send(o.CustomerEmail, confirmation(o))
    return nil
}

// Caller:
func placeOrder(o Order) error {
    if err := SaveAndNotify(o); err != nil {
        return err
    }
    metrics.Inc("orders.confirmed_and_notified")
    return nil
}

What's wrong? The order is saved. The customer swears they never got a confirmation email, yet the "confirmed_and_notified" metric keeps climbing and there are zero errors in the logs. Why?

func Save(o Order) error {
    return db.Save(o)
}

// Returns the notify error instead of hiding it. Caller chooses the policy.
func Notify(o Order) error {
    return notifier.Send(o.CustomerEmail, confirmation(o))
}

func placeOrder(o Order) error {
    if err := Save(o); err != nil {
        return err
    }
    metrics.Inc("orders.saved")

    if err := Notify(o); err != nil {
        // explicit best-effort: log it AND record that we failed to notify
        log.Printf("order %s saved but notify failed: %v", o.ID, err)
        metrics.Inc("orders.notify_failed")
        return nil // truly best-effort, but now it's visible and measured
    }
    metrics.Inc("orders.notified")
    return nil
}

Snippet 8 — Default mutable argument (Python)¶

Difficulty: Medium

def add_item(item, cart=[]):
    """Add item to cart and return the cart. Defaults to a fresh empty cart."""
    cart.append(item)
    return cart


# Two unrelated shoppers, two calls that each omit `cart`:
alice_cart = add_item("apple")
bob_cart = add_item("banana")

print(alice_cart)  # expected: ["apple"]
print(bob_cart)    # expected: ["banana"]

What's wrong? Alice and Bob each start with no cart and add one item. Both carts come back with two items. Why?

def add_item(item, cart=None):
    """Add item to cart and return it. A fresh cart is created when omitted."""
    if cart is None:
        cart = []
    cart.append(item)
    return cart

Snippet 9 — Boolean flag inverts on a refactor (Java)¶

Difficulty: Hard

public class CacheLoader {
    // Original: load(key, useCache). useCache=true means "read cache first".
    public Value load(String key, boolean useCache) {
        if (useCache) {
            Value cached = cache.get(key);
            if (cached != null) return cached;
        }
        Value fresh = source.fetch(key);
        cache.put(key, fresh);
        return fresh;
    }
}

// Six months later, someone "clarifies" the API by renaming the flag,
// because callers were confused about what `true` meant:
public Value load(String key, boolean bypassCache) {
    if (bypassCache) {
        Value cached = cache.get(key);
        if (cached != null) return cached;
    }
    Value fresh = source.fetch(key);
    cache.put(key, fresh);
    return fresh;
}

// Existing caller, unchanged through the rename:
Value v = loader.load("config", true);

What's wrong? Only the parameter name changed; the body is identical. The code compiles, all existing call sites are untouched. Yet caching behavior is now backwards. Why?

public enum CachePolicy { USE_CACHE, REFRESH }

public Value load(String key, CachePolicy policy) {
    if (policy == CachePolicy.USE_CACHE) {
        Value cached = cache.get(key);
        if (cached != null) return cached;
    }
    Value fresh = source.fetch(key);
    cache.put(key, fresh);
    return fresh;
}

// Or eliminate the parameter entirely:
public Value load(String key)        { /* USE_CACHE path */ }
public Value reload(String key)      { /* REFRESH path  */ }

Snippet 10 — Mixed abstraction levels hide a skipped guard (Go)¶

Difficulty: Hard

func transferFunds(from, to *Account, amount Money) error {
    // High-level intent mixed with low-level mechanics:
    fromLocked := tryLock(&from.mu)
    if !fromLocked {
        return ErrBusy
    }

    if err := authorize(from, amount); err != nil {
        unlock(&from.mu)
        return err
    }

    toLocked := tryLock(&to.mu)

    from.balance = from.balance.Sub(amount)
    to.balance = to.balance.Add(amount)

    unlock(&from.mu)
    unlock(&to.mu)
    return nil
}

What's wrong? This works in every test (single-threaded, accounts never contended). Under load, money is occasionally created or destroyed, and the service sometimes panics on unlock. Where are the bugs?

func transferFunds(from, to *Account, amount Money) error {
    release, err := lockBoth(from, to) // one place owns the locking protocol
    if err != nil {
        return err // could not acquire BOTH locks; nothing was touched
    }
    defer release()

    if err := authorize(from, amount); err != nil {
        return err
    }
    from.balance = from.balance.Sub(amount)
    to.balance = to.balance.Add(amount)
    return nil
}

// lockBoth acquires both mutexes (in a stable order to avoid deadlock) or none.
func lockBoth(a, b *Account) (release func(), err error) {
    first, second := orderByID(a, b) // consistent ordering prevents deadlock
    if !tryLock(&first.mu) {
        return nil, ErrBusy
    }
    if !tryLock(&second.mu) {
        unlock(&first.mu)
        return nil, ErrBusy
    }
    return func() { unlock(&second.mu); unlock(&first.mu) }, nil
}

Snippet 11 — Query method that mutates a cache, called under a read lock (Java)¶

Difficulty: Hard

public class PricingService {
    private final Map<String, BigDecimal> priceCache = new HashMap<>();
    private final ReadWriteLock lock = new ReentrantReadWriteLock();

    // Looks like a pure getter. Callers treat it as read-only.
    public BigDecimal getPrice(String sku) {
        BigDecimal cached = priceCache.get(sku);
        if (cached == null) {
            cached = remotePricing.fetch(sku);
            priceCache.put(sku, cached);   // (!) write inside a "getter"
        }
        return cached;
    }

    // Caller assumes getPrice is a read, so it grabs only the READ lock:
    public BigDecimal totalFor(List<String> skus) {
        lock.readLock().lock();
        try {
            return skus.stream()
                       .map(this::getPrice)
                       .reduce(BigDecimal.ZERO, BigDecimal::add);
        } finally {
            lock.readLock().unlock();
        }
    }
}

What's wrong? Under concurrency the price cache occasionally throws ConcurrentModificationException or returns corrupt totals, even though totalFor carefully takes a lock. Why isn't the lock protecting it?

public BigDecimal getPrice(String sku) {       // QUERY: never mutates
    lock.readLock().lock();
    try {
        BigDecimal cached = priceCache.get(sku);
        if (cached != null) return cached;
    } finally {
        lock.readLock().unlock();
    }
    return populateAndGet(sku);                 // miss path is explicit
}

private BigDecimal populateAndGet(String sku) { // COMMAND: takes write lock
    lock.writeLock().lock();
    try {
        return priceCache.computeIfAbsent(sku, remotePricing::fetch);
    } finally {
        lock.writeLock().unlock();
    }
}

Snippet 12 — Output parameter "filled in" only on the happy path (Python)¶

Difficulty: Medium

def parse_config(text: str, errors: list) -> dict:
    """Parse config text into a dict. Append any problems to `errors`."""
    result = {}
    for lineno, line in enumerate(text.splitlines(), start=1):
        line = line.strip()
        if not line or line.startswith("#"):
            continue
        if "=" not in line:
            errors.append(f"line {lineno}: missing '='")
            return result          # <-- early return on first error
        key, value = line.split("=", 1)
        result[key.strip()] = value.strip()
    return result


# Caller:
errors = []
config = parse_config(user_text, errors)
if errors:
    raise ConfigError("\n".join(errors))
apply(config)

What's wrong? The caller carefully checks errors and raises if there are any. Yet malformed configs with several mistakes only ever report one error, and sometimes a config with a typo gets partially applied. Why?

from dataclasses import dataclass, field

@dataclass(frozen=True)
class ParseResult:
    config: dict = field(default_factory=dict)
    errors: list[str] = field(default_factory=list)

    @property
    def ok(self) -> bool:
        return not self.errors


def parse_config(text: str) -> ParseResult:
    result, errors = {}, []
    for lineno, line in enumerate(text.splitlines(), start=1):
        line = line.strip()
        if not line or line.startswith("#"):
            continue
        if "=" not in line:
            errors.append(f"line {lineno}: missing '='")
            continue                      # collect ALL errors, don't bail
        key, value = line.split("=", 1)
        result[key.strip()] = value.strip()
    return ParseResult(config=result if not errors else {}, errors=errors)


# Caller can't apply a half-parsed config by accident:
outcome = parse_config(user_text)
if not outcome.ok:
    raise ConfigError("\n".join(outcome.errors))
apply(outcome.config)

Scorecard¶

Count the snippets where you named the bug and the function-design root cause before opening the answer.

The signal that matters is not "I found a bug" but "I can name the interface defect that made the bug possible" — flag argument, output parameter, temporal coupling, command-query violation, returned null, hidden side effect. Each name comes with a standard fix.

Related Topics¶

• README.md — the positive rules of function design this file inverts.
• junior.md — the beginner-level definitions of each anti-pattern shown here.
• tasks.md — exercises to refactor flag args, output params, and CQS violations out of real code.
• ../15-pure-functions — why hidden side effects (Snippets 2, 6) break referential transparency.
• ../06-error-handling — return special-case objects / Optional / Result instead of null (Snippets 5, 12).
• ../11-concurrency — command-query mixes turn into data races under locks (Snippets 4, 11).
• ../../anti-patterns/README.md — the broader catalogue of code-level traps these snippets belong to.
• ../../refactoring/README.md — the mechanical refactorings (Replace Flag with Methods, Introduce Parameter Object, Remove Output Parameter) that fix these bugs safely.