Code Review Workshop

Review three provided code samples and write review comments for each. The goal is fluency in the rubric (correctness, clarity, design, tests) and in the comment style (specific, constructive, evidence-based).

Retrieval Prompts

1. Name the four rubric buckets.
2. Give the shape of a good review comment.
3. What is the difference between a nit and a substantive comment?

Sample 1: The Quiet Bug

# src/checkout/totals.py
def compute_total(items, discount_pct=0):
    subtotal = 0
    for item in items:
        subtotal += item["price"] * item["qty"]
    if discount_pct:
        subtotal = subtotal - (subtotal * discount_pct / 100)
    tax = subtotal * 0.08
    return subtotal + tax

Review the following lenses; do not invent defects to satisfy a count. Distinguish contract questions from demonstrated failures:

• one [correctness, substantive]
• one [clarity, nit]
• one [design, substantive]
• one [tests, substantive]
• one comment that labels itself explicitly as non-blocking

Hints (don't read unless stuck): Python division here is not integer truncation. Ask about currency representation, valid discount bounds and tax policy; the snippet alone does not specify those contracts. Missing visible tests is a request for evidence, not proof that no tests exist.

Sample 2: The Cathedral of Patterns

public interface UserFetcher { User fetch(long id); }

public class DefaultUserFetcher implements UserFetcher {
    private final UserFetcherStrategy strategy;
    private final UserFetcherStrategyFactory factory;
    public DefaultUserFetcher(UserFetcherStrategyFactory factory) {
        this.factory = factory;
        this.strategy = factory.createDefault();
    }
    public User fetch(long id) {
        return strategy.fetch(id);
    }
}

public class UserFetcherStrategyFactory {
    public UserFetcherStrategy createDefault() {
        return new DatabaseUserFetcherStrategy();
    }
}

public class DatabaseUserFetcherStrategy implements UserFetcherStrategy {
    public User fetch(long id) {
        return Database.getUser(id);
    }
}

There is one implementation of UserFetcherStrategy. There are no callers doing anything besides fetcher.fetch(id).

Write review comments covering:

• which patterns are not pulling their weight (and which specific rule of simple design they violate)
• what the author should have done instead
• what you would merge and what would block

Sample 3: The Almost-Clean Feature

// src/promotions/apply.ts
export async function applyPromoCode(cartId: string, code: string) {
  const cart = await db.carts.get(cartId);
  const promo = await db.promotions.get(code);
  if (!promo) return { ok: false, error: "not_found" };
  if (promo.expiresAt < Date.now()) return { ok: false, error: "expired" };
  if (promo.uses >= promo.maxUses) return { ok: false, error: "exhausted" };
  const discount = promo.kind === "pct"
    ? cart.total * (promo.amount / 100)
    : promo.amount;
  promo.uses++;
  await db.promotions.put(promo);
  return { ok: true, newTotal: cart.total - discount, auditId: uuid() };
}

Write comments that cover:

• at least one concurrency bug (hint: race between two redemptions)
• at least one testability issue (Date.now(), uuid())
• at least one design comment about where this logic belongs
• at least one missing-test comment

Receiving Feedback Drill

For two of your own review comments, write the author's reply using the 5-step pattern (acknowledge, address, agree/modify/push-back, ask, move forward). Make at least one reply disagree constructively with your own comment.

Evidence Check

This page is complete when:

• You covered each sample with evidence-based findings or explicit questions; no minimum comment count.
• Each comment is labeled by bucket and nit/substantive.
• Each blocking finding cites a concrete trigger and consequence, with a test or other verification route.
• You practiced one disagreement reply.