Boy Scout Rule — Find the Bug¶

The Boy Scout Rule says: leave the campground cleaner than you found it. But there's a sharp edge. A "cleanup" is only safe if it is behavior-preserving. Below are 11 diffs, each presented as an innocent tidy-up — a renamed variable, a simplified condition, a deleted "dead" branch, a loop turned into a stream. Every one of them shipped a bug, because the author changed behavior while believing they were only changing form, and no test stood in the way. Find what each cleanup broke.

Table of Contents¶

1. How to Use
2. Snippet 1 — "Simplifying" a boolean condition (Java)
3. Snippet 2 — Deleting "dead" code reached by reflection (Java)
4. Snippet 3 — Loop → stream changes ordering (Java)
5. Snippet 4 — Extract Method drops a defer (Go)
6. Snippet 5 — Magic number → constant with the wrong value (Python)
7. Snippet 6 — Inlining a variable evaluated once (Python)
8. Snippet 7 — Tightening a type breaks a caller (Go)
9. Snippet 8 — Deleting a "redundant" null check (Java)
10. Snippet 9 — Rename collides with a shadowed variable (Python)
11. Snippet 10 — for → comprehension skips an edge case (Python)
12. Snippet 11 — De-duplicating two "identical" branches (Go)
13. Snippet 12 — "Obvious" early return drops a finally side effect (Java)
14. Scorecard
15. Related Topics

How to Use¶

Each snippet shows a before (the messy original) and a cleaned version that looks strictly better — shorter, clearer, more idiomatic. Your job is the reviewer's job: not "is this prettier?" but "is this the same program?"

For each one:

1. Read the cleaned diff and decide whether you'd approve the PR.
2. Write down what, if anything, changed about the behavior — not the style.
3. Open the answer and check: the behavior change, why the existing tests stayed green, and the safe way to land the same cleanup.

The thread running through all of them is in the diagram below: a clean-up is safe only when it sits inside the behavior-preserving box and has a test pinning the behavior down.

Snippet 1 — "Simplifying" a boolean condition (Java)¶

Difficulty: ★★☆☆☆

The original condition looked redundant. The cleanup applied De Morgan and dropped what seemed like a duplicate guard.

// BEFORE
public boolean isEligible(Account account) {
    if (account != null && account.isActive() && account.getBalance() > 0) {
        return true;
    }
    return false;
}

// CLEANED — "just return the expression, and the != null is implied by isActive()"
public boolean isEligible(Account account) {
    return account.isActive() && account.getBalance() > 0;
}

What did the cleanup break?

public boolean isEligible(Account account) {
    return account != null && account.isActive() && account.getBalance() > 0;
}

Snippet 2 — Deleting "dead" code reached by reflection (Java)¶

Difficulty: ★★★☆☆

The IDE flagged a method as unused. The cleanup deleted it and a "never instantiated" constructor.

// BEFORE
public class ReportJob {
    public ReportJob() { }                 // IDE: "no usages"
    public void run() { /* generate report */ }
    private void onComplete() {             // IDE: "no usages"
        metrics.increment("report.completed");
    }
}

// CLEANED — removed the unused no-arg constructor and the unused private method
public class ReportJob {
    public void run() { /* generate report */ }
}

What did the cleanup break?

Snippet 3 — Loop → stream changes ordering (Java)¶

Difficulty: ★★★☆☆

A verbose accumulation loop became a tidy stream pipeline.

// BEFORE
public List<String> topTags(List<Post> posts) {
    Map<String, Integer> counts = new LinkedHashMap<>();
    for (Post p : posts) {
        for (String tag : p.getTags()) {
            counts.merge(tag, 1, Integer::sum);
        }
    }
    List<String> result = new ArrayList<>();
    for (Map.Entry<String, Integer> e : counts.entrySet()) {
        if (e.getValue() >= 3) result.add(e.getKey());
    }
    return result;
}

// CLEANED — one expressive stream
public List<String> topTags(List<Post> posts) {
    return posts.stream()
        .flatMap(p -> p.getTags().stream())
        .collect(Collectors.groupingBy(t -> t, Collectors.counting()))
        .entrySet().stream()
        .filter(e -> e.getValue() >= 3)
        .map(Map.Entry::getKey)
        .collect(Collectors.toList());
}

What did the cleanup break?

.collect(Collectors.groupingBy(t -> t, LinkedHashMap::new, Collectors.counting()))

Snippet 4 — Extract Method drops a defer (Go)¶

Difficulty: ★★★☆☆

A long handler was tidied by extracting the locking-and-work part into a helper.

// BEFORE
func (c *Cache) Refresh(key string) error {
    c.mu.Lock()
    defer c.mu.Unlock()
    val, err := c.loader(key)
    if err != nil {
        return err
    }
    c.entries[key] = val
    return nil
}

// CLEANED — extracted the body into a focused helper
func (c *Cache) Refresh(key string) error {
    c.mu.Lock()
    return c.refreshLocked(key)
}

func (c *Cache) refreshLocked(key string) error {
    defer c.mu.Unlock()
    val, err := c.loader(key)
    if err != nil {
        return err
    }
    c.entries[key] = val
    return nil
}

What did the cleanup break?

func (c *Cache) Refresh(key string) error {
    c.mu.Lock()
    defer c.mu.Unlock()
    return c.refreshLocked(key) // helper assumes the lock is held; documented in its name + a comment
}

func (c *Cache) refreshLocked(key string) error {
    val, err := c.loader(key)
    if err != nil {
        return err
    }
    c.entries[key] = val
    return nil
}

Snippet 5 — Magic number → constant with the wrong value (Python)¶

Difficulty: ★★☆☆☆

A bare number was promoted to a named constant — exactly what the Boy Scout Rule encourages.

# BEFORE
def session_is_expired(last_seen: datetime, now: datetime) -> bool:
    return (now - last_seen).total_seconds() > 1800   # magic number


# CLEANED — name the magic number
SESSION_TIMEOUT_MINUTES = 30

def session_is_expired(last_seen: datetime, now: datetime) -> bool:
    return (now - last_seen).total_seconds() > SESSION_TIMEOUT_MINUTES

What did the cleanup break?

SESSION_TIMEOUT = timedelta(minutes=30)

def session_is_expired(last_seen: datetime, now: datetime) -> bool:
    return now - last_seen > SESSION_TIMEOUT

Snippet 6 — Inlining a variable evaluated once (Python)¶

Difficulty: ★★★☆☆

A "pointless" temporary variable used twice was inlined for brevity.

# BEFORE
def assign_ticket(queue):
    agent = pick_next_agent()          # mutates round-robin cursor, returns an agent
    log.info("assigning to %s", agent.name)
    queue.assign(agent.id)
    return agent

# CLEANED — the temporary is used only to read .name and .id; inline it
def assign_ticket(queue):
    log.info("assigning to %s", pick_next_agent().name)
    queue.assign(pick_next_agent().id)
    return pick_next_agent()

What did the cleanup break?

Snippet 7 — Tightening a type breaks a caller (Go)¶

Difficulty: ★★★☆☆

A function returned a broad interface{}; the cleanup tightened it to the concrete type actually returned.

// BEFORE
func Lookup(key string) (interface{}, bool) {
    v, ok := store[key]
    return v, ok
}

// CLEANED — "it always returns a *User, so say so"
func Lookup(key string) (*User, bool) {
    v, ok := store[key]
    if !ok {
        return nil, false
    }
    return v.(*User), false  // <- assert to the concrete type
}

What did the cleanup break?

Snippet 8 — Deleting a "redundant" null check (Java)¶

Difficulty: ★★☆☆☆

A null check looked redundant because the value was checked one line above.

// BEFORE
public String formatAddress(Customer customer) {
    Address addr = customer.getAddress();
    if (addr == null) {
        return "(no address)";
    }
    String city = addr.getCity();
    if (city == null) {                 // "redundant — addr isn't null"
        return "(no city)";
    }
    return city.toUpperCase();
}

// CLEANED — removed the second null check; addr is already non-null
public String formatAddress(Customer customer) {
    Address addr = customer.getAddress();
    if (addr == null) {
        return "(no address)";
    }
    return addr.getCity().toUpperCase();
}

What did the cleanup break?

Snippet 9 — Rename collides with a shadowed variable (Python)¶

Difficulty: ★★★★☆

A confusingly reused name was renamed for clarity — but the rename was applied too broadly.

# BEFORE
def summarize(records):
    data = load_defaults()          # outer: a dict of default fields
    for record in records:
        data = {**data, **record}   # inner reuse of `data` = merged row (intentional? buggy?)
        emit(data)
    return data                     # returns the LAST merged row's data

# CLEANED — "the name `data` is overloaded; rename the loop variable to `merged`"
def summarize(records):
    data = load_defaults()
    for record in records:
        merged = {**data, **record}   # renamed the loop-local
        emit(merged)
    return merged

What did the cleanup break?

Snippet 10 — for → comprehension skips an edge case (Python)¶

Difficulty: ★★★☆☆

An imperative parse loop became a tidy comprehension.

# BEFORE
def parse_amounts(lines):
    results = []
    for line in lines:
        line = line.strip()
        if not line:
            continue                 # skip blank lines
        if line.startswith("#"):
            continue                 # skip comments
        results.append(Decimal(line))
    return results

# CLEANED — one comprehension
def parse_amounts(lines):
    return [Decimal(line.strip()) for line in lines if not line.startswith("#")]

What did the cleanup break?

def parse_amounts(lines):
    stripped = (line.strip() for line in lines)
    return [Decimal(s) for s in stripped if s and not s.startswith("#")]

Snippet 11 — De-duplicating two "identical" branches (Go)¶

Difficulty: ★★★★☆

Two branches looked like copy-paste. The cleanup merged them under the Don't-Repeat-Yourself banner.

// BEFORE
func price(plan string, seats int) int {
    switch plan {
    case "team":
        base := 10 * seats
        if seats > 50 {
            base = base * 90 / 100 // 10% volume discount
        }
        return base
    case "business":
        base := 18 * seats
        if seats > 50 {
            base = base * 85 / 100 // 15% volume discount
        }
        return base
    }
    return 0
}

// CLEANED — extract the shared volume-discount logic
func price(plan string, seats int) int {
    rate := map[string]int{"team": 10, "business": 18}[plan]
    if rate == 0 {
        return 0
    }
    base := rate * seats
    if seats > 50 {
        base = base * 90 / 100 // volume discount
    }
    return base
}

What did the cleanup break?

var discountByPlan = map[string]struct{ rate, volumeMul int }{
    "team":     {10, 90},
    "business": {18, 85},
}

func price(plan string, seats int) int {
    p, ok := discountByPlan[plan]
    if !ok {
        return 0
    }
    base := p.rate * seats
    if seats > 50 {
        base = base * p.volumeMul / 100
    }
    return base
}

Snippet 12 — "Obvious" early return drops a finally side effect (Java)¶

Difficulty: ★★★★☆

A method had a try/finally that "wrapped the whole body for no reason." The cleanup added an early return for the empty case.

// BEFORE
public Result process(List<Job> jobs) {
    span.start();
    try {
        Result r = new Result();
        for (Job j : jobs) {
            r.merge(run(j));
        }
        return r;
    } finally {
        span.end();   // always closes the tracing span
    }
}

// CLEANED — bail out early when there's nothing to do
public Result process(List<Job> jobs) {
    span.start();
    if (jobs.isEmpty()) {
        return Result.empty();   // fast path
    }
    try {
        Result r = new Result();
        for (Job j : jobs) {
            r.merge(run(j));
        }
        return r;
    } finally {
        span.end();
    }
}

What did the cleanup break?

public Result process(List<Job> jobs) {
    span.start();
    try {
        if (jobs.isEmpty()) {
            return Result.empty();   // inside try → finally still runs
        }
        Result r = new Result();
        for (Job j : jobs) {
            r.merge(run(j));
        }
        return r;
    } finally {
        span.end();
    }
}

Scorecard¶

Rate yourself by how you'd have caught each one as a reviewer — not by whether you eventually understood the answer.

Caught 10–12: you read for behavior, not aesthetics. You instinctively ask "what input made the messy version messy?"

Caught 6–9: solid reviewer reflexes. Sharpen the habit of demanding a characterization test before approving any "pure cleanup."

Caught 0–5: the lesson landed where it counts. The fix is not "be smarter" — it's process: no cleanup ships without a behavior-pinning test, and cleanups land in their own commit, separate from features.

The single rule behind all twelve: a cleanup is only a cleanup if it is behavior-preserving and test-protected. The moment it changes an observable — an ordering, a null path, a side effect, a constant, a number of evaluations — it is a behavior change wearing a cleanup's clothes, and it deserves the same scrutiny, tests, and separate review as any other behavior change. That is the cautionary other half of the Boy Scout Rule: leave it cleaner, but leave it doing the same thing — and prove both.

Related Topics¶

• junior.md — the plain-language definition of the Boy Scout Rule and its limits
• tasks.md — practice exercises: turn unsafe cleanups into safe, test-protected steps
• Boy Scout Rule — chapter README — the positive rules and the anti-patterns to avoid
• Refactoring — the discipline of behavior-preserving change, with characterization tests
• Anti-Patterns — the broader catalog of well-intentioned changes that go wrong