Code Review and Refactoring

The seat that two people can reserve

You are asked to review this endpoint. The interviewer tells you only that user_id belongs to an authenticated user and that db is a simple data access object.

reserved = {}

def reserve_seat(event_id, user_id, seat_id):
    if user_id in reserved:
        return {"ok": False, "error": "user already has a seat"}

    event = db.get_event(event_id)
    if not event:
        return {"ok": False, "error": "event not found"}

    seat = db.get_seat(event_id, seat_id)
    if seat["status"] == "reserved":
        return {"ok": False, "error": "seat reserved"}

    db.update_seat(event_id, seat_id, {"status": "reserved", "user": user_id})
    reserved[user_id] = seat_id
    return {"ok": True, "seat": seat_id}

Take a minute before reading on. What promise does this function appear to make? Which failure would you discuss first? What evidence in the code supports your concern, and what test would expose it?

A code review round is not a hunt for the greatest number of comments. It asks whether you can discover the behavior that matters, find the implementation choices that endanger it, and propose a change whose safety can be inspected. The most consequential line in this example is not badly named or badly formatted. It is the distance between reading a seat and writing it.

Reconstruct the promise

The signature and success response suggest a provisional contract: an authenticated user may reserve an available seat for an event, and a successful response means that the reservation now belongs to that user. Two users must not both receive success for the same seat.

Some parts of the contract cannot be inferred honestly. Does the product allow one seat per user overall or one per event? Must the user be eligible for this event or seat class? What should a repeated request from the same user return? Can a closed sale be changed while a request is in flight? Are the free-text errors already part of a public API?

Those are review questions, not excuses to stop. State the minimum promise you can defend, name the uncertainties that could change the repair, and proceed. For example:

“I read success as a durable claim that this user now owns this seat and no competing request can also succeed. I need to clarify whether the one-seat limit is global or per event, and whether eligibility must be checked here.”

That statement gives the review a standard. Without it, “globals are bad” is a preference. With it, process-local memory is evidence that the implementation cannot enforce a durable rule across workers.

Contract reconstruction also determines what not to criticize. If the function is internal and its callers already translate exceptions, a new error hierarchy may be unnecessary. If the service handles a dozen reservations a minute, a few database calls may be acceptable. A risk earns attention through the promise and constraints, not because it appears on a memorized checklist.

Find the first broken promise

Imagine two requests for seat A1. Both call db.get_seat before either calls db.update_seat. Both observe "available"; both write; both return success. The later write may hide the earlier one in storage, but it cannot retract the success already returned to the first caller.

That interleaving breaks the central promise, so it belongs first. A useful review comment joins consequence, evidence, and a repair boundary:

“Two requests can both reserve the same seat because availability is read and updated in separate operations. Please make the availability predicate and ownership write one transaction or conditional update, and cover the losing request with a concurrency or conditional-write test.”

The comment is firm without pretending that the author was careless. It tells them what can happen, where the evidence is, and what would prove the repair.

The global reserved map is another high-risk defect, but for a different reason. Its name implies authority that it does not have. Each server process owns a different map; a restart forgets it; and the key omits event_id. Depending on the intended contract, it either blocks a user from reserving at two different events or fails to block the same user when requests reach two workers. The database is already the durable coordination boundary, so this rule belongs there too.

After those failures, the missing-seat path deserves attention. If db.get_seat returns None, indexing seat["status"] raises an exception. The caller may see a generic server error even though “seat not found” is a normal domain outcome. This is less catastrophic than double-selling a seat, but it is concrete, reproducible, and worth fixing.

Authentication leaves an important question unanswered. It proves who the user is, not whether the user may reserve this event, during this sale window, or in this seat class. Do not invent an authorization policy that the prompt never supplied. Identify the trust boundary and ask who owns the decision. If this endpoint is the authoritative write path, a UI-only eligibility check is not enough.

By now the ordering is visible:

1. Prevent two successful owners for one seat.
2. Move any one-seat-per-user rule out of process-local memory.
3. Handle absent records and define stable caller-visible failures.
4. Clarify and enforce eligibility at the authoritative boundary.

Naming and file layout can wait. They do not threaten the promise in front of you.

Turn each concern into a proof

“Needs more tests” gives the author no design information. A diagnostic test states the rule that the implementation currently fails to protect.

Before changing code, pin the agreed behavior:

def test_missing_seat_returns_a_stable_error():
    assert reserve_seat("event-1", "user-1", "missing") == {
        "ok": False,
        "code": "SEAT_NOT_FOUND",
    }

def test_a_user_can_reserve_at_two_events():
    assert reserve_seat("event-1", "user-1", "A1")["ok"] is True
    assert reserve_seat("event-2", "user-1", "B1")["ok"] is True

def test_only_one_request_wins_a_seat():
    first, second = reserve_concurrently(
        ("event-1", "user-1", "A1"),
        ("event-1", "user-2", "A1"),
    )
    assert sorted(result["ok"] for result in (first, second)) == [False, True]

These tests assume that the one-seat limit is per event and that the API is moving to stable error codes. If either choice is unsettled, say so. A characterization test can preserve existing caller-visible behavior while the team decides; a contract test can describe an intentional correction once the decision is made. Calling every change “behavior preserving” does not make it so.

The concurrency test is ideal when the harness can control the interleaving. When it cannot, test the storage primitive directly: seed one available seat, perform two conditional writes, and assert that exactly one row was claimed. The proof should live as close as possible to the boundary that owns the rule.

Move the rule to its owner

A safe refactor does not begin by reorganizing the function. First make the ordinary failure paths explicit:

def reserve_seat(event_id, user_id, seat_id):
    event = db.get_event(event_id)
    if event is None:
        return error("EVENT_NOT_FOUND")

    seat = db.get_seat(event_id, seat_id)
    if seat is None:
        return error("SEAT_NOT_FOUND")

    if not db.user_can_reserve(event_id, user_id, seat_id):
        return error("NOT_ELIGIBLE")

    # The ownership write is still unsafe. That is the next boundary to move.

This step improves the contract but does not fix the race. Say that aloud. A review loses credibility when an intermediate cleanup is presented as a correctness repair.

Next, give the database one operation that owns the decision and the write:

def reserve_seat(event_id, user_id, seat_id):
    validation_error = validate_reservation_request(event_id, user_id, seat_id)
    if validation_error:
        return validation_error

    outcome = db.try_reserve_available_seat(event_id, seat_id, user_id)
    if outcome == "SEAT_UNAVAILABLE":
        return error("SEAT_UNAVAILABLE")
    if outcome == "USER_ALREADY_RESERVED":
        return error("USER_ALREADY_RESERVED")

    return {"ok": True, "seat": seat_id}

The helper name is not the design. Its contract is: claim the seat only if it is still available, enforce the agreed uniqueness rules, and report which constraint prevented the claim. That may be implemented with a transaction, a conditional update, or database constraints. If eligibility can change concurrently, the authoritative operation must also recheck the relevant eligibility state rather than trusting an earlier read.

Only after this operation passes the diagnostic tests should the local reserved map disappear. At each checkpoint, the public behavior is either unchanged or the intended contract change is explicit. That is the discipline that separates refactoring from rewriting.

Let the remaining risks earn their place

The example contains several familiar review lenses, but they are not equally important in every prompt.

Performance becomes relevant when a constraint makes it observable. Three database round trips may be acceptable for one interactive reservation; a database call per seat in a bulk allocation path may create timeouts and load. Ask what grows and where the budget is before recommending a cache or a batch API. A cache added to the current design could make ownership harder to reason about, not faster in any useful sense.

Error handling belongs to the contract because callers act on outcomes. A timeout after the database commits is especially important: the caller does not know whether retrying will create a second action. An idempotency key or a lookup by request identifier may be necessary, but only if retries are part of the operating model. The review should distinguish “rejected,” “not found,” “already reserved,” and “outcome unknown” when those states demand different caller behavior.

Security belongs near the top when code touches identity, payments, personal data, files, secrets, or privileged actions. Name the boundary and the missing decision. “Check security” is no more useful than “add tests.” Here the useful question is whether the durable reservation write can be reached without an authoritative eligibility decision.

Coupling matters when a rule is owned invisibly. The global map, an omitted event identifier in a cache key, tests that depend on execution order, or a helper that mutates caller-owned state all create behavior the call site cannot see. Name the dependency and the failure it permits.

Abstraction comes last unless structure itself hides the rule. A SeatReservationManager that merely forwards these calls adds vocabulary without improving ownership. The atomic storage operation earns a name because it protects a real invariant. Extract a domain rule when doing so makes the rule harder to violate; stop when the next layer would only make the code look more designed.

Make the reasoning audible

In a live review, the interviewer does not need every observation. They need to hear why your ordering is trustworthy. A compact exchange might sound like this:

This is collaborative without being vague. Severity comes from consequence, not forceful wording. Evidence makes the criticism useful.

Practice on one resistant example

Choose a short service function with a database or network dependency. Give yourself twenty minutes. First write the contract you can infer and the two questions whose answers would change the implementation. Then produce only three review comments, ordered by the failure you would most want to prevent. For each comment, name the input, interleaving, dependency failure, or trust boundary that proves the risk.

Spend the remaining time on one refactor. Preserve the current contract with a test, move one rule to a visible owner, and rerun the proof. If the code needs an intentional behavior change, separate that decision from the structural change. Do not award yourself points for comment volume. Ask whether an author could act on each comment without guessing what danger you meant.

Testing begins from known cases; debugging follows a known contradiction. Review starts earlier, when the failures are still latent. Its central act is to reconstruct the promise, identify the implementation choice most capable of breaking it, and make the repair small enough that the promise remains visible throughout the change.

Related reading

• Chapter 34: Writing Senior-Level Interview Code
• Chapter 35: Testing in the Interview
• Chapter 36: Debugging Live
• Chapter 38: Complete Annotated Coding Interviews
• Chapter 57: Working in an Unfamiliar Codebase