How Do You Conduct a Code Review?

Prompt and Applicable Context

You have 30 minutes to review an unfamiliar pull request. Explain how you reconstruct its intent, order the review, separate blockers from non-blocking feedback, write comments the author can act on, and choose among approve, comment, and request changes.

This is a general software-engineering interview question for backend, frontend, mobile, infrastructure, and engineering-management roles. It may be asked as a verbal process question or as a live review of a supplied diff. Both forms test whether you can find the issues that matter most to users and the system under a time limit, rather than maximizing the number of formatting faults you report.

Assume you can see the pull-request description, linked requirement, changed files, and test results, but you do not know the codebase and cannot continuously question the author. If the interviewer supplies different conditions, recalibrate the risk and scope before reviewing.

What the Interviewer Evaluates

The first signal is whether you reconstruct what the change is supposed to do. Without a requirement, API contract, or failure boundary, comments reduce to preference. A strong answer reads the pull-request context and surrounding code, identifies the behavior change, and only then goes line by line.

The second signal is prioritization. Correctness, data corruption, security, authorization, concurrency, and compatibility generally deserve attention before naming or layout. The interviewer wants to see whether you can state the consequence and spend time on the highest-risk path.

The third signal is evidence. “This might be a bug” is only a lead. High-quality feedback gives the triggering condition, observable consequence, and a way to verify it. When context is missing, it asks a precise question instead of disguising a guess as a blocking conclusion.

Finally, the interviewer evaluates the review decision and communication. Separate required fixes, optional suggestions, clarifying questions, and nits. In the summary, state what you covered, what remains unverified, and why you approved or requested changes. Discuss the code and its impact, not the author's ability.

Questions to Clarify Before Answering

• Is this a verbal process question or a live diff review? For the former, present a reusable method. For the latter, spend a few seconds stating the method and then apply it to actual lines instead of reciting a checklist.
• What context is available? A requirement, API contract, and surrounding code let you verify behavior. With an isolated function, state assumptions and turn unknown contracts into questions.
• Does the change touch a high-risk domain? Payments, authorization, privacy, migrations, and public APIs raise the evidence bar and may require domain experts. A low-risk internal tool can favor faster incremental improvement.
• Which deliverables does the interviewer expect? Inline comments, a summary, an approval decision, and test recommendations require different time allocations. Confirm the output before spending the 30 minutes.
• Is this normal work or an emergency fix? An emergency can justify a narrower patch and follow-up work. It does not justify ignoring a known security or data-corruption risk.
• Are you qualified for every affected domain? If the change involves cryptography, privacy, or a database migration outside your expertise, review what you can and request a qualified reviewer rather than approving on confidence.

30-Second Answer Framework

“I first establish the pull request's goal, behavior change, and failure impact, then review in two passes. Pass one maps the change boundary, data flow, and high-risk paths, prioritizing correctness, security, data, concurrency, and compatibility. Pass two checks edge cases, error handling, tests, observability, performance, and maintainability. Each comment states severity, trigger, consequence, and expected result. A reproducible blocker means request changes; suggestions and nits can accompany approval. I finish with the reviewed scope, unverified risks, and the reason for my decision.”

Step-by-Step Deep Answer

Establish the baseline first. Read the title, description, linked requirement, API or data-model changes, and existing tests. Restate the contract in one sentence: “This change gives this user a new behavior under these conditions while preserving these existing guarantees.” If you cannot write that sentence, acquire context before line-level comments because you do not yet have a standard of correctness.

Map the change boundary next. Follow inputs, state changes, external side effects, and return paths rather than only highlighted lines. Which callers receive a new parameter? Are a database write and message send inside the same failure boundary? Did a public response, event format, or configuration default change? The output of this pass is a model of where input enters, which trust boundaries it crosses, what state changes, and how it can fail.

For a sample 30-minute time box, use 3 minutes for intent, 7 minutes for boundaries and high-risk paths, 12 minutes for detailed inspection, 5 minutes for tests and operational safeguards, and 3 minutes for comments and the decision. This is a practice allocation to adjust for diff size and risk. Its purpose is to prevent spending the first 20 minutes on naming.

Use this risk order in the first pass:

1. Behavior and correctness: Does the main path satisfy the contract? What happens with empty input, duplicate requests, partial failure, and retries?
2. Security and data: Does authorization occur before crossing the trust boundary? Is sensitive data exposed? Can failure cause loss, duplication, or irreversible state?
3. Concurrency and compatibility: Can simultaneous requests break an invariant? Do old clients, old data, and mixed versions during a rolling deployment still work?
4. Architecture boundary: Is the responsibility in the right component, or does the change bypass an existing constraint and duplicate state?

The second pass examines implementation detail: control flow and error propagation, resource cleanup, query or loop scale, logs and metrics, whether tests would actually fail when the code is wrong, and whether names and comments help future readers. Put formatting and automatically fixable style last so tool-detectable nits do not displace human judgment.

For each finding, verify that the change introduced or exposed it. If new code reads items[0] when empty items are valid, that is a concrete regression. If the same file contains unrelated pre-existing complexity, mention it as debt or create follow-up work unless its interaction with this change creates a security or correctness risk. A review cannot expand without a boundary.

Use four comment intents:

• Blocker: Evidence shows a contract violation, incorrect result, security issue, data corruption, or unacceptable compatibility risk. It must be resolved before merge.
• Question: Context that could change the conclusion is missing. The answer may close the concern or promote it to a Blocker.
• Suggestion: A design, maintainability, or operational improvement that is worthwhile, while the current implementation still meets the merge bar.
• Nit: A non-blocking readability or consistency detail that formatting or static analysis should ideally handle.

An actionable comment contains “label + condition + consequence + expected result,” followed by one possible direction when useful. For example:

Blocker: When the request permits items=[], reading items[0].id here throws and the batch endpoint returns 500. Please handle the empty array before the loop and add a regression test; whether to return an empty result or 400 depends on the API contract.

If you do not know whether empty input is legal, make it a question: “Does the API contract allow an empty array? The current path returns 500; if it is allowed, it needs explicit handling and a test.” This reports the evidence without inventing a requirement.

Finish with a review decision. Select request changes when a Blocker is unresolved. Submit a comment when essential context is missing rather than hiding uncertainty behind approval. Approve when only non-blocking suggestions remain, and state that they are not merge conditions. The summary should name the reviewed scope, key findings, runtime or test evidence, uncovered domains, and final status.

Green CI does not prove that a review is complete. Tests may omit the critical branch, and static tools do not know the product contract. Conversely, human review should not replace executable tests. Connect the two: identify a failing condition in the comment and request a check that fails before the fix and passes afterward.

High-Quality Sample Answer

“I would not begin by hunting line-level faults. I would read the pull-request description, linked requirement, and interface changes, then restate the goal and the old behavior that must remain true. If context is unavailable, I would list assumptions instead of presenting guesses as blockers.

Within 30 minutes, I would use two passes. The first follows entry points, state changes, external side effects, and return paths, prioritizing correctness, security, data corruption, concurrency, and compatibility. The second covers edge cases, error handling, performance, logs, tests, and maintainability. Style comes last, and only when tooling did not cover an issue that actually affects comprehension.

Every finding must answer three questions: what triggers it, what the consequence is, and how to verify it. I label required fixes Blocker, missing context Question, non-blocking improvements Suggestion, and polish Nit. For example, if an empty-array path reads the first element even though the API accepts empty input, I would explain that it returns 500 and request explicit handling plus a regression test, rather than writing only ‘possible null issue.’

Before submitting, I check that each comment relates to this diff, that I did not elevate preference into a rule, and that no required domain reviewer is missing. A reproducible security, data, or correctness problem leads to request changes; suggestions alone can accompany approval. My summary lists the files covered, test evidence, unverified areas, and decision rationale so the author knows the next action and later reviewers know what I actually reviewed.”

Common Mistakes

• Opening the diff and commenting line by line → Without intent or a contract, valid tradeoffs look like faults → Restate the goal, behavior change, and failure boundary first.
• Commenting in discovery order → Naming details can bury data-corruption or authorization risks → Run the risk pass before implementation polish.
• Writing only “this might be a bug” → The author lacks a trigger and cannot verify the fix → State the condition, consequence, evidence, and expected result.
• Making every comment mandatory → The author cannot distinguish the merge bar from preference → Label Blocker, Question, Suggestion, and Nit explicitly.
• Reciting a long checklist to appear thorough → A list not applied to data flow demonstrates little judgment → Trace one critical path and explain the remaining coverage.
• Demanding that all old problems be fixed → The pull request expands without bounded risk or validation → Separate regressions from existing debt except where they combine into a safety or correctness issue.
• Treating green CI as approval evidence → Tests may encode the wrong contract or omit a branch → Check whether they fail for the key counterexample and request the missing regression test.
• Commenting on the author instead of the code → It creates defensiveness and supplies no technical rationale → Describe the code, condition, and impact while assuming good intent.
• Approving outside your expertise → The approval creates false assurance → State your coverage and request a domain reviewer.

Follow-Up Questions and Responses

Follow-up 1: What if the author disputes your Blocker?

Return to the verifiable contract and consequence. Check whether you disagree about inputs, risk, or release conditions, and use a minimum reproduction when possible. If evidence does not produce consensus, ask the code owner or domain owner to decide and record any verbal conclusion in the pull request. Do not let disagreement remain indefinitely.

Follow-up 2: What if the pull request is too large to finish in 30 minutes?

Do not imply full coverage. Select entry points, data changes, and public contracts by risk; state which files you reviewed line by line, scanned, or did not cover; then request a split or additional domain reviewers. The approval status must match the coverage actually performed.

Follow-up 3: What if you find a severe pre-existing issue outside the diff?

First determine whether this change triggers or amplifies it. Block when the interaction creates a security, data, or correctness risk for the current release. If it is independent, record evidence, create high-priority follow-up work, and notify the owner instead of forcing an unbounded refactor into this pull request.

Follow-up 4: Can you approve an emergency fix without complete tests?

Establish the immediate cost of not fixing, whether the patch is narrow, the rollback or feature-switch path, and the smallest targeted validation available. An explicit emergency process may accept follow-up coverage, but a known security, data-corruption, or irreversible risk still needs a higher approval bar. The clock does not approve it automatically.

Follow-up 5: How do you review a domain you do not know?

Continue checking general control flow, error handling, tests, and interface changes while marking what you are unqualified to judge. Request the appropriate owner for cryptography, privacy, migrations, or complex concurrency. A partial review is valuable only when it is not represented as complete approval.

Follow-up 6: Do you still need to read every line when test coverage is extensive?

Yes, with a different focus. Tests provide executable evidence for encoded cases; review still asks whether the requirement is correct, risks are missing, the design adds unnecessary complexity, and logging or compatibility is appropriate. Turn important counterexamples found in review into tests so future correctness does not depend on reviewer memory.