Learn

In a code review round, lead with the defect that hurts someone

15 October 2026 · Code review: find it, then defend it

The interviewer shares a small pull request and says "review this as you would at work". You find five things in two minutes and read them out in the order you found them. A naming nit comes first, the real bug comes fourth, and by the time you reach it the interviewer has already written down that you review line by line.

The situation

A review round is not scored on how many findings you list. It is scored on whether you can tell which one matters, and whether you can show why with something more solid than "this feels risky". Interviewers who run this round say much the same thing: a short list in the right order beats a long list in reading order.

Here is an original example. A service checks whether a user may delete a document. Before the change, every call ran one database query for the user's roles. The pull request title is "Cache roles to cut query load", and this is the new version:

_roles = {}  # user_id -> set of role names

def can_delete(user_id, doc):
    roles = _roles.get(user_id)
    if roles is None:
        roles = load_roles(user_id)  # one database query
        _roles[user_id] = roles
    return "admin" in roles or doc.owner_id == user_id

It works. Every test that existed before still passes, and the query count drops. That is exactly why it is a good review question.

What goes wrong, step by step

There are three findings here. Read them, then decide the order before you read on.

• Stale permissions. The cache never expires. Walk it through with a concrete sequence: user 7 is an admin and calls can_delete, so {"admin"} is cached. An administrator removes the role in the database. User 7 calls can_delete again, the cache answers, and the function returns True until the process restarts. If several worker processes run, each one keeps its own copy, so the same user may be allowed on one request and refused on the next.
• Unbounded growth. Every user who ever calls the function stays in the dictionary. On a service with a large and changing user base, memory grows for as long as the process lives.
• A string literal. "admin" appears inline. A shared constant would be tidier.

The order is not close. To rank a finding, ask three questions and answer each with evidence from the diff, not from instinct:

1. Who gets hurt? Stale permissions hurt whoever owns the documents, and the case it breaks, revoking access, is the reason the check exists at all. Growth hurts operations, slowly. The literal hurts nobody.
2. When does it show? Stale permissions show only after a revoke, which no existing test performs, so it will pass review and CI and appear in production. Growth shows on a dashboard over days. The literal never shows.
3. Can it be undone? A deleted document may not come back. A restarted process frees its memory. A literal can be renamed any time.

That gives you the order: block on the first, ask for a follow-up on the second, mention the third as optional or not at all.

What to say out loud

In the room, lead with the blocking finding and give the sequence that proves it: "I would block on this one. Step one: user 7 is an admin, so their roles are cached. Step two: someone removes the role. Step three: the same call still returns true, because nothing ever expires the cache. This is an authorisation check, so a revoke has to take effect. I would either not cache here, or agree a short expiry and say out loud how long a removed admin may keep acting. The unbounded dictionary is real but slower, so I would want a size limit as a follow-up. The inline string is a nit; I would not hold the change for it."

Notice what that does. The interviewer hears the severity, the evidence and a fix, and you have also asked the question that decides the design: how long may a revoked user keep their rights? If the answer is "never", the cache does not belong in this function.

The exercise

1. Open a recent pull request of your own or a colleague's, ideally under 50 changed lines.
2. Write down every finding you can, without ranking. Give yourself five minutes.
3. For each one, answer the three questions: who gets hurt, when does it show, can it be undone. One line each.
4. For the top finding, write the shortest concrete sequence of calls or inputs that makes it happen, the way user 7 does above.
5. Say the top finding aloud in under thirty seconds: severity, evidence, fix. Then say which findings you would drop entirely.

Where this fits in Gadfly

Gadfly has four rooms: algorithms, system design, behavioral and code review. The code review room takes a class you wrote yourself, a connected GitHub repository, or a project from its catalog, and reviews it together with the transcript of how the code was produced. It reads and discusses the code; it does not execute it, so a sequence like the one above is something you reason through aloud. Code review is part of a session pack. You code in Java or Python, and the tutor speaks English.