Code Review and Working with Existing Codebases Questions
Reviewing others' code and navigating unfamiliar systems: giving and receiving actionable review feedback, spotting correctness and design issues, and reading and understanding large or legacy codebases before changing them. Covers collaborative coding norms, incremental change in shared repositories, and verifying changes against existing behavior. The team-facing side of day-to-day engineering.
As part of a code review, you are asked to ensure that a change respects the team's API stability policy. Describe how you would verify backwards compatibility, what kind of tests you'd want in place, and how you'd document and communicate an intentional breaking change to downstream consumers.
Sample Answer
Direct answer
Treat this review as three linked jobs: prove the change doesn't break the existing contract, or clearly flag that it intentionally does; have tests that catch a break automatically rather than relying on a human noticing during review; and if it IS a breaking change, run an explicit communication and coordination process across every downstream team before it ships, not just a changelog entry after the fact.
Structured elaboration
Verify backward compatibility. Diff the public API (application programming interface) surface, endpoints, request and response schemas, public method signatures, against the previous version using an automated schema-diff tool. Check that the semantic-versioning bump (version numbers like 2.1.0, where the first number only increases for a breaking change) actually matches the impact: a removed field or endpoint is a major-version break, not a minor one. Manually scan for the classic breaking patterns an automated diff might present too dryly to flag on its own: a field that used to be optional becoming required, a removed enum value, or a changed error code that a consumer's error-handling logic depends on.
Tests to require. Unit and integration tests for the new behavior, plus consumer-driven contract tests: each known consumer publishes a "contract" describing what it expects from this API, and the provider's continuous integration (CI) pipeline runs every consumer's contract against the new code before merge, so a break shows up as a failing build instead of a production incident weeks later.
Documenting and communicating an intentional break. If a break is genuinely necessary, it needs a migration guide with concrete before-and-after request or response examples, a deprecation timeline (announce now, remove after an agreed grace period), and a version bump that signals the change to anyone watching semantic versioning.
The multi-team angle. This is the part that's easy to skip. Different downstream teams often run on different release cadences: one team deploys daily, while another ships a mobile client that goes through app-store review on a multi-week cycle. "We announced it in the changelog" is not the same as "every consumer had a realistic window to migrate before the old behavior disappeared." The reviewer's job includes checking that the deprecation window is long enough for the SLOWEST consumer's release cadence, not the average one, and that there's a direct, tracked notification, not just a changelog line, to every known downstream team: who owns each consumer, has that owner acknowledged the timeline, and is there a release-coordination step, such as a shared rollout calendar or a required sign-off from each consuming team, before the old behavior is actually removed.
Worked example
A concrete scenario: a payments API removes a deprecated legacy_currency_code field. Applying the process: an automated schema diff confirms the field is gone, which requires a major-version bump. Contract tests run for the three known consumers (a web checkout, a mobile app, and a partner integration), and the partner integration's contract still references the old field, so its test currently fails, that failure is the exact signal that should hold the release, not something to work around. A migration guide is written showing the old request shape next to the new one. The deprecation timeline is set to match the partner integration's slowest release cadence, which, per that team, needs six weeks of notice because their deploys go through a change-advisory process, not the web team's daily-deploy pace. Each of the three consuming teams gets a direct message naming the field, the deadline, and a link to the migration guide, and the field's actual removal is gated on each team explicitly acknowledging the change, not merely being notified of it.
Trade-offs and pitfalls
Coordinating across three teams' release cadences is slower than shipping the break and hoping consumers keep up, but the alternative, a downstream team's production system silently breaking because nobody saw a changelog entry, is far more expensive to unwind afterward. A common pitfall is confusing "we deprecated it" with "it's now safe to remove": the deprecation period only did its job if every consumer actually acted on it, which needs active follow-up, for example a dashboard tracking remaining callers of the old field, rather than passively waiting out the clock.
You are reviewing a new REST endpoint PR. Describe a complete test strategy to validate behavior, including unit tests, integration tests, contract tests, load tests, and edge case tests. For each test type give a concrete example case that must be covered and what the reviewer should verify in the test code.
Sample Answer
Direct answer
Reviewing a new REST (Representational State Transfer, the common web API style) endpoint's test coverage means checking five distinct layers: unit tests for the endpoint's own logic in isolation, integration tests against real dependencies, contract tests that pin the API's shape for consumers, load tests for realistic traffic, and explicit edge-case tests. Each layer catches a different class of bug, and none of them substitutes for the others.
Structured elaboration
| Test type | Concrete example case | What the reviewer verifies |
|---|---|---|
| Unit tests | Given a valid request body, the handler returns the expected response shape and status code; given an invalid field, it returns a 400 with a specific error message | The test exercises the real logic, not just that a mock was called, and covers both the success path and at least one validation failure |
| Integration tests | Create a resource via the endpoint, then confirm it's actually persisted and retrievable via a separate read call | The test uses a real or realistic data layer, not mocks, so it would catch a broken query or a schema mismatch |
| Contract tests | A field a known consumer depends on, e.g. createdAt, must always be present and always an ISO 8601 timestamp string; the test fails if a future change removes or reshapes it | The contract covers every field an actual known consumer depends on, not just a generic happy-path shape check |
| Load tests | Simulate the expected peak requests-per-second and confirm the p95 latency, the 95th-percentile response time, meaning 95% of requests are at least this fast, stays under the team's target with near-zero error rate | The load profile reflects realistic expected traffic, and the test checks a latency or error threshold, not only that requests completed |
| Edge case tests | An empty request body, a request at the maximum allowed payload size, a duplicate submission with the same idempotency key (a client-generated ID sent with a request so retrying it doesn't create a duplicate), a request from an unauthenticated caller | Each case has an explicit assertion about expected behavior, a specific status code and body, not just "doesn't throw an unhandled exception" |
Worked example
A PR (pull request) adds POST /orders. Unit test: a valid order payload returns 201 with an order ID; a missing items field returns 400. Integration test: after the POST, a subsequent GET /orders/{id} returns the same order from the real test database. Contract test: the response always includes id, status, and createdAt in the shape the frontend's order-confirmation page depends on. Load test: simulate the expected peak checkout traffic and confirm p95 latency and error rate stay within the team's targets. Edge cases: an empty items array, a duplicate submission with the same idempotency key that should not create two orders, and a request with an expired auth token that should return 401, not 500.
Trade-offs and pitfalls
Not every endpoint needs all five layers at full weight, a low-traffic internal admin endpoint may not need load testing, and a purely internal endpoint with no external consumers may not need a formal contract test. Calibrate to the endpoint's actual risk and consumers rather than mechanically requiring every category on every PR. A common wrong turn is treating a high unit-test count as sufficient while skipping integration tests entirely; unit tests with everything mocked can pass while the real database query underneath is broken.
As a software engineer, explain your ideal code review process for pull requests under 200 lines. Describe reviewer selection, a minimal checklist (style, correctness, tests, security), ownership expectations, acceptable turnaround time, and how to distinguish blocking from non-blocking comments.
Sample Answer
Direct answer
A good process for a pull request (PR, a proposed set of code changes submitted for review before merging) under 200 lines is fast and lightweight: one clear reviewer, a short checklist covering style, correctness, tests, and security, a same-day turnaround, and comments explicitly labeled as blocking or non-blocking so the author never has to guess what must change before merge.
Structured elaboration
Reviewer selection
- Route to the primary owner of that code area (via a CODEOWNERS file or team convention), not automatically to the most senior person, who otherwise becomes a bottleneck.
- For anything touching a public interface, add a second reviewer who is unfamiliar with that area specifically because they will catch assumptions the primary owner has stopped noticing.
Minimal checklist
- Style: does it match team conventions? Push mechanical disagreements to an automatic formatter/linter, not a person's opinion, so the review thread never argues about spacing.
- Correctness: does the change do what the description says, including realistic edge cases (empty input, a null value, an error path)?
- Tests: is the new or changed behavior actually asserted by a test, not just executed by one?
- Security: any hardcoded secret, unvalidated input crossing a trust boundary, or a newly added dependency worth a second look?
- For a backend change specifically: how will we know this worked in production, e.g. a new log line or metric tied to the change, not just "does it compile."
Ownership expectations
The author owns getting the change mergeable; the reviewer owns the technical bar, not the implementation details. Whoever approves shares responsibility for what ships, so an approval should mean "I understood this," not "I skimmed it."
Turnaround time
Same business day for a first pass on anything this small. The review itself is process overhead sitting on top of the actual work, so keeping it fast protects the whole point of using small PRs.
Blocking vs. non-blocking
A blocking comment is one that, if ignored, breaks correctness, security, or leaves the codebase worse; it must be resolved before merge. A non-blocking comment is a preference, an alternative approach, or a nice-to-have, and should be labeled explicitly (a "nit:" prefix works well) so the author isn't left guessing.
Worked example
A 120-line PR adds a retry to a payment API call. The primary owner reviews it same-day. Checklist pass: correctness looks fine on the happy path, but the retry catches a bare exception instead of the specific timeout error, which would silently swallow a real bug. Reviewer comment: "this catches every exception including a genuine failure, which will hide bugs. Please catch the specific timeout exception instead." That's blocking. A second comment, "nit: could inline this variable if you want, up to you," is non-blocking. The PR description already states an acceptance criterion the reviewer checks against: "done = a transient network failure retries up to 3 times with backoff and logs a warning, verified by the new unit test."
Trade-offs and pitfalls
Applying full scrutiny to a trivial one-line PR wastes everyone's time; scale depth to risk, not PR size alone. Skipping tests to "keep the PR small" defeats the purpose small PRs are supposed to serve, fast and safe iteration, not fast and unverified iteration. A common wrong turn: phrasing a nitpick in a blocking tone, which reads as stricter than intended and quietly damages trust between author and reviewer.
A PR changes logic that affects billing calculations. As a reviewer, list specific review checks you would perform to ensure correctness and compliance, including unit tests with edge cases, integration tests with realistic data, auditability of changes, and approvals required from finance or product stakeholders.
Sample Answer
Direct answer
Review a billing-logic change on two tracks at once: prove the math is correct with edge-case unit tests and realistic integration tests, and confirm the process around it is sound, meaning every calculation is auditable after the fact and the right stakeholders (finance and/or product) signed off before merge. A billing bug is a compliance and trust problem, not just a bug, so "the tests pass" is necessary but not sufficient here.
Structured elaboration
Correctness of the calculation itself
Check the rounding rule (round half-up vs. banker's rounding, where a tied value rounds to the nearest even number instead of always rounding up, and whether it's applied consistently), currency handling (money should use a fixed-point or decimal type, never plain floating-point, since floats can't represent amounts like 0.10 exactly), proration logic (charging only for the fraction of a billing period actually used), and timezone handling for anything date-dependent like a billing cycle boundary.
Unit tests with edge cases
Request tests covering: a zero amount, a negative amount (a refund or credit), an amount that lands exactly on a rounding boundary, missing or null optional fields (a discount that wasn't applied, a tax rate that doesn't exist for a region), and an extremely large value. Each of these is a place where a plausible-looking formula quietly breaks.
Integration tests with realistic data
A unit test proves the formula is right in isolation; an integration test proves the whole pipeline (ingestion, calculation, invoice generation, ledger posting) produces the right result end to end. Run it against data that resembles production, meaning multiple plans, discounts, promo codes, and tax rules together, not just the one clean case a unit test isolates.
Auditability
Every calculation should leave a trail: which code version produced it, what the inputs were, and what the output was, structured and logged in a way that isn't overwritten. Without this, nobody can answer "why was this specific customer's invoice this amount" six months later, which is exactly the question support and finance will eventually ask.
Approvals from finance or product
Engineering approval on the code is not the same as confirmation that the business rule itself is correct. A rounding rule, a proration policy, or a tax treatment is a decision finance owns, and a discount or pricing rule is one product owns; the PR (pull request) should not merge without an explicit sign-off from whichever of those the change actually touches, recorded on the ticket, not assumed from a hallway conversation.
Worked example
An illustrative review comment tying this together for a hypothetical mid-cycle-upgrade proration change:
"Before I approve this: (1) can you add unit tests for a customer upgrading with exactly half the billing cycle remaining, a customer upgrading on the last day of the cycle, and a customer with a discount active during the upgrade; (2) can you run this against a sanitized copy of real account data covering a few different plan and discount combinations, not just the one example in the PR description; (3) does each recalculated invoice log the inputs and the code version that produced it, so we can explain a specific number to a customer later; (4) has Finance actually reviewed the new proration formula, since this changes what customers get charged, not just how the code is organized?"
An illustrative structured log line the reviewer would want to see land alongside the change (values shown are illustrative, not a real invoice): {"invoice_id": "inv_123", "calc_version": "v3", "inputs": {"plan": "pro", "days_remaining": 15, "cycle_days": 30}, "output_cents": 1499}. The point isn't the exact schema, it's that the inputs and the version are captured at all.
Trade-offs and pitfalls
Requiring a finance or product sign-off slows the PR down, that's the correct trade for a change that determines what customers get charged; the cost of a wrong sign-off skipped once is much higher than the delay of getting it every time. The most common pitfall is testing the math correctly (good unit tests) while skipping the process controls, since a technically-correct calculation that nobody can explain later, or that nobody with business authority actually approved, is still a compliance gap. A second common pitfall is testing only against synthetic numbers a developer invented, which tend to avoid exactly the messy combinations (discount plus proration plus a currency conversion) that show up in real accounts.
Behavioral: Tell me about a time when you found a critical bug or security issue in infrastructure code during a code review. Use the STAR format: describe the Situation, the Task you had, the Actions you took as reviewer and with the team, and the Results (including any follow-up changes to process or automation).
Sample Answer
Direct answer
I'll walk through a real example from reviewing infrastructure-as-code: catching a security group change that would have opened a database to unrestricted inbound access, and how that turned into both an immediate fix and a lasting change to how the team reviews that class of change.
Structured elaboration
Situation. I was reviewing a routine-looking Terraform PR meant to let a new internal reporting service reach a database. Task. As the reviewer, my job was to catch anything that changed the actual security posture of that database, not just check that the Terraform plan applied cleanly. Action. I noticed the PR's security group rule used 0.0.0.0/0 for the inbound CIDR range (CIDR, Classless Inter-Domain Routing, is the notation for writing a whole block of IP addresses as one value; 0.0.0.0/0 specifically means "every possible IP address"), instead of the reporting service's specific subnet, almost certainly copy-pasted from an example rather than deliberately chosen. Result covers what happened next, both the immediate fix and the longer-term process change, below.
Worked example
I marked the PR as blocking with a specific comment explaining the exposure: this rule would allow any host on the internet to attempt a connection to the database's port, not just the internal reporting service the PR was supposedly scoping access to. I proposed the concrete fix, scoping the rule to the reporting service's actual subnet CIDR instead, and pushed a one-line diff to make it easy for the author to just take. Given the severity, I also flagged it in the team's on-call channel rather than waiting for an asynchronous PR reply, since an already-merged version of a similar mistake elsewhere in the account was worth checking for immediately, not after the PR conversation finished. The PR was updated and merged with the corrected, scoped rule within the hour. Separately, I proposed and helped add an automated policy check (using Open Policy Agent, a policy-as-code tool that can evaluate Terraform plans against written rules) to the CI pipeline that specifically rejects any security group rule opening a sensitive port to 0.0.0.0/0 without an explicit, reviewed exception, so this exact mistake can't reach production again without a deliberate override.
Trade-offs and pitfalls
The judgment call in a story like this is deciding how loudly to escalate: raising it in a live channel instead of just a PR comment was the right call given the actual exposure, but that same urgency would be the wrong tone for a much lower-severity finding, and using it there would just train the team to tune out urgent-sounding messages. A pitfall to watch for when telling this kind of story is stopping at "I found the bug and it got fixed," without the automation follow-up: the more convincing version of this story is one where the process change means the same category of mistake gets caught automatically next time, not just this one instance.
Unlock Full Question Bank
Get access to all Code Review and Working with Existing Codebases interview questions and detailed answers.
Sign in to ContinueJoin thousands of developers preparing for their dream job.