Code Review Interviews

You are given a pull request and twenty minutes. Two things are being scored simultaneously: what you find, and how you say it.

The code

# orders.py — under review
from typing import List, Dict

def process_orders(orders: List[Dict], customers: Dict[int, str], min_amount=0):
    """Return total revenue per country for completed orders above min_amount."""
    results = {}
    for order in orders:
        if order['status'] == 'completed':
            if order['amount'] > min_amount:
                country = customers[order['customer_id']]
                if country in results:
                    results[country] += order['amount']
                else:
                    results[country] = order['amount']

    sorted_results = sorted(results.items(), key=lambda x: x[1], reverse=True)

    output = ""
    for country, total in sorted_results:
        output += country + ": " + str(round(total, 2)) + "\n"

    return output

Read it before continuing. There are more problems than there appear to be.

Read in this order

1. SIGNATURE + DOCSTRING   what does it promise? does the return type match?
2. DATA FLOW               follow one record through, start to finish
3. ERROR PATHS             what raises? what is silently swallowed?
4. BOUNDARIES              empty, one, duplicates, null, largest realistic
5. CONCURRENCY / STATE     shared mutable state? ordering assumptions?
6. STYLE                   last, and marked as minor

Announce the order — “let me read the signature first, then trace one record through” — because a systematic search is itself scored, and it stops you fixating on the first thing you spot.

Step 1: the signature contradicts the docstring

def process_orders(orders: List[Dict], customers: Dict[int, str], min_amount=0):
    """Return total revenue per country..."""
    ...
    return output          # a string

“The docstring says it returns totals per country, and the return type is an unannotated string of formatted lines. That is a presentation concern inside a calculation function — a caller that wants to do anything with the numbers has to parse them back out. I’d return Dict[str, Decimal] and format at the call site. This is the finding I’d lead with, because it is the one that shapes the rest.”

Leading with the design issue rather than a missing null check is a seniority signal.

Step 2: trace one record

orders = [
    {"order_id": 1, "customer_id": 1, "status": "completed", "amount": 25.50},
    {"order_id": 2, "customer_id": 9, "status": "completed", "amount": 19.99},  # unknown customer
    {"order_id": 3, "customer_id": 1, "status": "Completed", "amount": 40.00},  # capital C
    {"order_id": 4, "customer_id": 2, "status": "completed", "amount": 0.00},   # exactly zero
]
customers = {1: "GB", 2: "US"}

try:
    print(process_orders(orders, customers))
except KeyError as e:
    print(f"KeyError: {e}")

KeyError: 9

Bug 1 — unhandled missing key. customers[order['customer_id']] raises on any order whose customer is not in the dict, which takes down the whole batch for one bad row.

    country = customers.get(order["customer_id"], "UNKNOWN")

“Whether the right behaviour is UNKNOWN, skipping with a count, or raising is a product question — but crashing the whole batch on one orphan is almost certainly not it. I’d ask what the caller expects and, either way, the count of orphans should be visible rather than silent.”

Remove that row and run again:

print(process_orders(orders[:1] + orders[2:], customers))

GB: 25.5

Bug 2 — 'Completed' with a capital C was silently dropped. The £40 order never counted.

“Exact string comparison on a status field. If the data comes from more than one source, or a human, the casing will vary. order['status'].strip().lower() == 'completed' handles it — though if statuses are a closed set, an enum at the boundary would be better than normalising at every use site.”

Bug 3 — the min_amount boundary. Order 4 has amount == 0.00 and min_amount defaults to 0, so > min_amount excludes it. Is that intended?

“> versus >= with a default of 0 means a genuinely zero-value order is excluded. That might be right — a zero order may not be revenue — but it is not stated anywhere, and the default makes it look accidental. This is a question rather than a bug: what should happen at exactly min_amount?”

Flagging ambiguity as a question rather than declaring it a bug is the correct register, and interviewers notice.

Step 3: the remaining defects

def process_orders(orders, customers, min_amount=0):
    ...
    if order['status'] == 'completed':
        if order['amount'] > min_amount:

Bug 4 — missing keys on the order itself. order['status'] and order['amount'] both raise KeyError on a malformed record. Same class as bug 1, different dictionary.

Bug 5 — floats for money.

total = 0.0
for amount in [0.1, 0.2, 0.3, 25.50, 12.00]:
    total += amount
print(f"float:   {total!r}")

from decimal import Decimal
total_d = sum(Decimal(str(a)) for a in ["0.1", "0.2", "0.3", "25.50", "12.00"])
print(f"Decimal: {total_d}")

float:   38.1
Decimal: 38.10

print(f"0.1 + 0.2 == 0.3 → {0.1 + 0.2 == 0.3}")
print(f"0.1 + 0.2       → {0.1 + 0.2!r}")

0.1 + 0.2 == 0.3 → False
0.1 + 0.2       → 0.30000000000000004

“round(total, 2) at the end hides accumulated error rather than preventing it. On a few rows it will not matter; across millions of orders the drift becomes a reconciliation problem with finance. Decimal for money, or integer pence. This one is worth raising even though it will not show up in the tests.”

Bug 6 — string concatenation in a loop.

import time
rows = [("GB", 1234.56)] * 40_000

t0 = time.perf_counter()
out = ""
for c, t in rows:
    out += c + ": " + str(t) + "\n"
t1 = time.perf_counter()

joined = "\n".join(f"{c}: {t}" for c, t in rows) + "\n"
t2 = time.perf_counter()

print(f"concat  {t1-t0:7.4f}s")
print(f"join    {t2-t1:7.4f}s   ({(t1-t0)/(t2-t1):.0f}x faster)")

concat   0.8104s
join     0.0041s   (198x faster)

“Strings are immutable, so += copies the accumulated string each iteration — that is O(n²). It is invisible on ten countries and it is a genuine problem if this ever runs over per-customer rows. join is both faster and clearer. Minor here given the likely size, so I’d mark it as such — but it is a habit worth flagging.”

Bug 7 — the sort is not deterministic on ties. Two countries with identical totals come back in whatever order sorted happened to produce, which makes the output unstable across runs and awkward to test. key=lambda x: (-x[1], x[0]).

Bug 8 — no tests in the diff. Worth saying plainly and without edge: “I’d want a test for the missing-customer case and one for the empty input before this merges.”

The rewrite

from decimal import Decimal
from typing import Iterable, Mapping

COMPLETED = "completed"

def revenue_by_country(
    orders: Iterable[Mapping],
    customers: Mapping[int, str],
    min_amount: Decimal = Decimal("0"),
) -> tuple[dict[str, Decimal], int]:
    """Total revenue per country for completed orders at or above min_amount.

    Returns (totals, skipped_unknown_customer). Orders whose customer is not in
    `customers` are attributed to "UNKNOWN" and counted, never dropped silently.
    """
    totals: dict[str, Decimal] = {}
    unknown = 0

    for order in orders:
        status = str(order.get("status", "")).strip().lower()
        if status != COMPLETED:
            continue

        amount = Decimal(str(order.get("amount", 0)))
        if amount < min_amount:
            continue

        customer_id = order.get("customer_id")
        country = customers.get(customer_id)
        if country is None:
            country, unknown = "UNKNOWN", unknown + 1

        totals[country] = totals.get(country, Decimal("0")) + amount

    return totals, unknown


def format_totals(totals: Mapping[str, Decimal]) -> str:
    """Presentation, separated from calculation."""
    ordered = sorted(totals.items(), key=lambda kv: (-kv[1], kv[0]))   # deterministic
    return "\n".join(f"{country}: {total:.2f}" for country, total in ordered)

totals, unknown = revenue_by_country(orders, customers)
print(format_totals(totals))
print(f"\nunknown-customer orders: {unknown}")
print(f"\nempty input: {revenue_by_country([], customers)}")

GB: 65.50
UNKNOWN: 19.99
US: 0.00

unknown-customer orders: 1

empty input: ({}, 0)

All four orders now counted: the capital-C one included, the orphan visible rather than fatal, the zero-amount one included because >= is now explicit, and the totals exact.

How you say it is half the score

weak:    "This is wrong."
weak:    "Why did you do it this way?"          (reads as an accusation)
weak:    "Obviously this will crash."           ("obviously" is never helpful)

strong:  "Line 12 will raise KeyError if an order references a customer that
          isn't in the map — I hit that with the sample data. Is crashing the
          batch the intended behaviour, or should those be attributed to
          UNKNOWN and counted?"

The strong version has four parts worth copying: specific location, what happens, evidence you ran it, and a question rather than a verdict. It is also how you would want to receive the same finding.

Separate severity explicitly, because prioritisation is what the round reveals:

BLOCKING     KeyError on unknown customer — crashes the batch
BLOCKING     case-sensitive status — silently drops valid revenue
DISCUSS      float for money — drift at scale; Decimal or integer pence
DISCUSS      >= vs > at min_amount — what should happen exactly at the boundary?
NON-BLOCKING string concat in a loop — O(n²), immaterial at this size
NON-BLOCKING non-deterministic tie order — makes tests flaky
NON-BLOCKING returns a formatted string — I'd split calculation from presentation

A reviewer who marks everything blocking is as unhelpful as one who marks nothing.

The eight classes worth hunting

1. Missing key / null        dict[...] vs .get(), unchecked Optional
2. Boundary                  empty, one element, exactly the threshold, off-by-one
3. Type                      float for money, int division, silent str/int coercion
4. Silent data loss          inner join, exact string match, except: pass
5. Error handling            bare except, swallowed exception, no context in the message
6. Complexity                nested loops, `in` on a list, string += in a loop
7. Determinism               unstable sort, set iteration order, dict ordering assumptions
8. Concurrency               shared mutable state, check-then-act races, mutable default args

The mutable default argument is a classic plant:

def add_item(item, basket=[]):        # created ONCE, at definition time
    basket.append(item)
    return basket

print(add_item("a"))
print(add_item("b"))                  # not a fresh list

['a']
['a', 'b']

Say what you checked and found sound

"Things I checked that look right: the aggregation logic itself is correct, the
 sort direction matches the docstring, and there's no mutable default argument.
 The type hints on the parameters are accurate — it's only the return that's
 missing one."

This is worth thirty seconds. It shows the search was systematic rather than lucky, and it gives the interviewer evidence for the “did they read it properly” row even where you found nothing.

The scoring

Practice

KeyError: 9

GB: 25.5      (the £40.00 'Completed' order silently missing)

concat 0.8104s   join 0.0041s   (198x)

"Line 12 raises KeyError if an order references a customer that isn't in the map
 — I hit that with the sample data. Is crashing the batch intended, or should
 those be attributed to UNKNOWN and counted?"

Next: reading and negotiating the offer.