Code Reviews — Practice Tasks¶

13 review exercises: you are handed a PR, a diff, or a comment thread and asked to review it like a senior engineer would. Every task has a full model answer with reasoning. Difficulty climbs from "label these three comments" to "untangle a four-round review fight." The skill being trained is judgment — knowing what to flag, how to say it, and when to stop typing and start a call.

Table of Contents¶

1. Task 1 — Label three review comments (nit / suggestion / blocker) · Easy
2. Task 2 — Write three good comments on a diff · Easy
3. Task 3 — Rewrite a rude comment to be kind and specific · Easy
4. Task 4 — Spot the blocker a nitpicky review would miss · Medium
5. Task 5 — Turn "I'd do it differently" into actionable or approve · Medium
6. Task 6 — Write a clear PR description for a given diff · Medium
7. Task 7 — Approve / approve-with-nits / request-changes, justified · Medium
8. Task 8 — Split a mega-PR into a sensible stack · Hard
9. Task 9 — De-escalate a review ping-pong · Hard
10. Task 10 — The defensive author: respond without arguing · Hard
11. Task 11 — Review a diff with a hidden security blocker · Hard
12. Task 12 — Bikeshedding after the formatter · Medium
13. Task 13 — Full review of a real PR (open-ended) · Hard

How to Use¶

For each task: read the scenario, then write your review before opening the solution. A review is not a feeling ("this looks off") — it is a set of concrete comments, each with a severity and a suggested action. Hold yourself to that bar.

Three labels recur throughout. They are the vocabulary of a healthy review culture:

• blocker — the PR must not merge as-is. A correctness bug, a security hole, data loss, a broken contract, or code so unclear it will rot. You are saying "no" and you owe a reason.
• suggestion — a real improvement the author should weigh. Not merge-blocking on its own; the author may accept, defer, or decline with a reason.
• nit — cosmetic or trivial. Prefix it nit: so the author knows it is optional. If a formatter or linter could catch it, it should not be a human comment at all.

The golden rule: review the code, not the coder. Every comment should survive being read aloud to the author's face.

Task 1 — Label three review comments (nit / suggestion / blocker)¶

Difficulty: Easy

You are skimming a teammate's PR and have drafted three comments. Before posting, label each one nit, suggestion, or blocker, and decide whether the PR can be approved.

The diff (Python):

def parse_config(path: str) -> dict:
    with open(path) as f:
        data = json.load(f)
    timeout = data["timeout"]          # KeyError if missing
    retries = data.get("retries", 3)
    return {"timeout": timeout, "retries": retries}

Your draft comments:

1. "f is a vague name — call it config_file."
2. "data['timeout'] will raise KeyError with no context if the key is absent. A user with a typo in their config gets an opaque stack trace. Can we default it or raise a clear error?"
3. "Could batch these into a single data.get helper to reduce repetition."

Label each, then decide: approve, approve-with-nits, or request-changes?

Task 2 — Write three good comments on a diff¶

Difficulty: Easy

Write three review comments on the diff below — one targeting correctness, one targeting design, one targeting readability. Label each with a severity. Each comment must be specific enough that the author knows exactly what to change.

The diff (Go):

func (s *UserService) GetUsers(ids []int) []*User {
    var users []*User
    for _, id := range ids {
        u, _ := s.repo.FindByID(id)
        users = append(users, u)
    }
    return users
}

Task 3 — Rewrite a rude comment to be kind and specific¶

Difficulty: Easy

A reviewer left this comment on a PR:

"This is a mess. Did you even test it? Nobody writes loops like this anymore. Use a map."

The underlying technical point is valid — the code uses a linear scan inside a loop where a lookup map would be O(1). Rewrite the comment so it is kind, specific, and actionable, without losing the technical substance.

The code in question (Java):

for (Order order : orders) {
    for (Customer c : customers) {
        if (c.getId().equals(order.getCustomerId())) {
            order.setCustomerName(c.getName());
        }
    }
}

Map<String, Customer> byId = customers.stream()
    .collect(toMap(Customer::getId, c -> c));
for (Order order : orders) {
    Customer c = byId.get(order.getCustomerId());
    if (c != null) {
        order.setCustomerName(c.getName());
    }
}

Task 4 — Spot the blocker a nitpicky review would miss¶

Difficulty: Medium

A junior reviewer left four comments on the PR below, all about style. Read the diff, decide whether their comments are worth posting, and find the blocker they missed.

The diff (Python — a money-transfer endpoint):

def transfer(from_acct, to_acct, amount):
    if amount <= 0:
        raise ValueError("amount must be positive")
    from_balance = db.get_balance(from_acct)
    if from_balance < amount:
        raise InsufficientFunds()
    db.set_balance(from_acct, from_balance - amount)
    to_balance = db.get_balance(to_acct)
    db.set_balance(to_acct, to_balance + amount)
    return True

The junior's comments:

1. "from_acct should be from_account — don't abbreviate."
2. "Add a docstring."
3. "return True is redundant, the function could return None."
4. "Type hints are missing."

Task 5 — Turn "I'd do it differently" into actionable or approve¶

Difficulty: Medium

A reviewer left only this comment and selected Request Changes:

"Hmm, I would have used a strategy pattern here instead of the switch statement. I'd do it differently."

The author is now blocked and confused. The diff works, has tests, and ships a feature on a deadline. The code (Java):

double price(PlanType type, int seats) {
    switch (type) {
        case FREE:       return 0;
        case TEAM:       return seats * 8.0;
        case BUSINESS:   return seats * 15.0;
        case ENTERPRISE: return seats * 25.0;
        default: throw new IllegalArgumentException("unknown plan: " + type);
    }
}

Your job: as the reviewer, rewrite your own comment. Either make it a concrete, actionable request or convert it to an approval. Decide which, and justify it.

Task 6 — Write a clear PR description for a given diff¶

Difficulty: Medium

You inherited a branch from a teammate who left this PR description:

"fixes"

The diff does the following (you've read it): it changes the rate limiter from a fixed-window counter to a sliding-window log, adds a Redis-backed store so limits are shared across instances, raises the default limit from 60 to 100 req/min, and adds a Retry-After header to 429 responses. There is also a config flag RATE_LIMIT_STRATEGY to roll back to the old behavior.

Write a PR description a reviewer can actually use.

Task 7 — Approve / approve-with-nits / request-changes, justified¶

Difficulty: Medium

For each of the three small diffs below, decide: approve, approve-with-nits, or request-changes — and justify in one or two sentences.

Diff A (Go):

func IsEven(n int) bool {
    return n%2 == 0   // negatives: -4 % 2 == 0 in Go, fine
}

Diff B (Python):

def get_user(user_id):
    return cache.get(user_id) or db.query(User).filter_by(id=user_id).first()

Diff C (Java):

public void deleteAccount(long userId) {
    auditLog.record("delete", userId);
    accountRepo.deleteById(userId);
}

cached = cache.get(user_id)
if cached is not None:
    return cached
return db.query(User).filter_by(id=user_id).first()

Task 8 — Split a mega-PR into a sensible stack¶

Difficulty: Hard

A teammate opens a 1,400-line PR titled "Add notifications." Reviewing it as one unit is hopeless — no one can hold 1,400 lines in their head and catch real bugs. From the file list and description, it contains:

• A new notifications DB table + migration.
• A Notification model and repository.
• A generic Sender interface with EmailSender and SmsSender implementations.
• A Twilio client wrapper (for SMS) and a SendGrid client wrapper (for email).
• A NotificationService that decides which sender to use.
• Wiring it into 6 existing endpoints (order shipped, password reset, etc.).
• A feature flag NOTIFICATIONS_ENABLED.
• Config, secrets handling for the two API keys, and 40 tests.

Propose how to split this into a reviewable stack of PRs. Specify the order and what makes each one independently safe to merge.

Task 9 — De-escalate a review ping-pong¶

Difficulty: Hard

A PR has been open for 5 days and four review rounds. Here is the abridged thread on one comment:

Reviewer (round 1): I don't think this retry logic is right — it'll retry on 4xx errors too, which won't succeed.

Author (round 1): It only retries on the errors in RETRYABLE. 400 isn't in there.

Reviewer (round 2): But RETRYABLE includes ConnectionError, and our HTTP client raises ConnectionError for some 4xx responses behind the proxy. So it can still retry a 400.

Author (round 2): That seems like a client bug, not mine. Maybe. I'm not sure that actually happens.

Reviewer (round 3): I'm fairly sure it does, I saw it last month. Can you add a check?

Author (round 3): Added a status-code check. Does this work?

Reviewer (round 4): Now it won't retry on a genuine connection drop that has no status code. Hmm.

It's now round 5 and you are the author. What do you do?

Task 10 — The defensive author: respond without arguing¶

Difficulty: Hard

You are the author. A reviewer left this on your PR:

blocker: This function mutates the config argument in place (config["debug"] = True). The caller passes a shared config dict, so this silently changes behavior for every other component reading the same dict. Can we return a modified copy instead?

Your honest first reaction is defensive — you did it in place deliberately to avoid copying a large dict, and you think the reviewer is being paranoid. Write the response that a senior engineer sends. Then write the response to avoid.

Task 11 — Review a diff with a hidden security blocker¶

Difficulty: Hard

Review this PR that adds a "search users by name" admin endpoint. Find every issue and assign severity. There is at least one blocker.

The diff (Python / Flask + raw SQL):

@app.route("/admin/users/search")
def search_users():
    name = request.args.get("name", "")
    query = f"SELECT id, email, ssn FROM users WHERE name LIKE '%{name}%'"
    rows = db.execute(query).fetchall()
    return jsonify([dict(r) for r in rows])

query = "SELECT id, email FROM users WHERE name LIKE :pattern"
rows = db.execute(query, {"pattern": f"%{name}%"}).fetchall()

Task 12 — Bikeshedding after the formatter¶

Difficulty: Medium

The repo has gofmt (or Prettier / Black) running in CI; no PR merges unless it's formatted. On a 3-line bug-fix PR, a reviewer posts six comments:

1. "I prefer if err != nil { return err } on one line, not three."
2. "Can we align these struct field comments?"
3. "I'd use i instead of idx here."
4. "Two blank lines before this function would read better."
5. "Single quotes vs double quotes — pick one." (the formatter already enforces this)
6. "Move the import for errors above fmt." (the formatter already sorts imports)

The actual bug fix in the PR is correct. As a reviewer or tech lead seeing this thread, what do you do?

Task 13 — Full review of a real PR (open-ended)¶

Difficulty: Hard

Conduct a complete review of the PR below. Produce: (a) a verdict (approve / approve-with-nits / request-changes), (b) every comment with a severity label, and (c) one sentence of overall feedback to the author. Be thorough but not nitpicky.

PR title: "Add endpoint to export user's orders as CSV"

The diff (Python / Flask):

@app.route("/users/<user_id>/orders/export")
def export_orders(user_id):
    orders = db.query(Order).filter_by(user_id=user_id).all()
    output = "id,date,total\n"
    for o in orders:
        output += f"{o.id},{o.date},{o.total}\n"
    resp = make_response(output)
    resp.headers["Content-Type"] = "text/csv"
    return resp

import csv, io
buf = io.StringIO()
writer = csv.writer(buf)
writer.writerow(["id", "date", "total"])
for o in orders:
    writer.writerow([o.id, o.date.isoformat(), o.total])
output = buf.getvalue()

Self-Assessment¶

Score yourself after completing the tasks. You're reviewing at a senior level when you can consistently say "yes" to all of these:

• I label comments nit / suggestion / blocker and I'm calibrated — most comments are nits or suggestions, and I reserve "request-changes" for code that is actually wrong, unsafe, or unmaintainable.
• I check correctness, security, and trust boundaries before style. I find the atomicity bug and the injection hole, not just the variable name.
• Every comment I write describes the code and its consequences, never the coder. My comments would survive being read aloud to the author.
• I never block a working, tested PR on "I'd do it differently." I either approve or write a labeled, exampled suggestion.
• When I'm the author, I concede the valid part first, share my context as information rather than justification, and convert disagreement into a concrete decision.
• I recognize a mega-PR as unreviewable and propose a dependency-ordered stack with the behavior change isolated in the smallest unit.
• I switch to a call after ~two non-converging rounds — especially when the disagreement is about unknown facts — and I always write the outcome back in the thread.
• I never comment on what a formatter or linter already enforces, and I push settle-able preferences into tool config instead of relitigating them per PR.
• I can write a PR description that front-loads the why, the trade-offs, and the things hiding in the diff — so the reviewer reviews intent, not guesses it.

If any box is unchecked, re-do the matching task and articulate the principle in your own words.

Related Topics¶

• junior.md — the foundational definitions of the review anti-patterns practiced here.
• find-bug.md — diffs where a review-relevant bug hides; train the eye that Task 4 and Task 11 demand.
• optimize.md — when a review comment is genuinely about performance, not preference.
• chapter README — the positive rules of code review that these exercises invert and apply.
• refactoring practice — many review suggestions ("extract this," "this is duplicated") are refactorings; this is the vocabulary to name them precisely.