Code review questions.

76 code review questions from the bank, ready to practice out loud.

Learn the ideas first · free with an account

STAR and CARL: structuring your story Review comments that land

All 76 questions

Timestamp to date conversionA teammate's pull request adds a function that converts a list of timestamps into a list of dates by truncating the time portion. Walk me through what you'd check before approving.CorrectnessMid–senior

Reviewing HTTP retry logicA PR introduces a function that retries failed HTTP requests up to 3 times. Walk me through the edge cases you'd push on.Edge casesMid–senior

Feedback on dense single functionA PR adds 200 lines of valid but dense code into a single function. The reviewer feels something's off but can't articulate it. How would you give the feedback?ReadabilityMid–senior

Pushing back on happy-path-only testsA PR adds a substantial new feature but its tests only cover the happy path. How do you frame the feedback?Test coverageMid–senior

Reviewing a large-scale variable renameA PR renames a long-standing variable from data to records. The rename is technically clearer but touches 300 files. How would you respond?NamingMid–senior

Reviewing broad try-catch error handlingA PR wraps every external API call in a try-catch that logs and returns null on error. Walk me through what's worth raising.Error handlingMid–senior

Spotting bug from senior engineerYou're reviewing a PR from a senior engineer and you've spotted a real bug. How do you frame the comment?Feedback deliveryEntry–mid

Reviewing parallel stream filteringA PR adds a function that filters a list using parallel streams. Walk me through what subtle issues you'd look for.CorrectnessSenior–staff+

Sorting with nullable fieldsA PR sorts a list of users by 'last_active'. The field is sometimes null. What questions do you ask?Edge casesEntry–mid

Clever one-liner vs utilityA PR uses a clever one-liner instead of an existing utility function the team agreed on. The one-liner is shorter. What do you say?ReadabilityMid–senior

Untested code change scopeA PR adds a small change that requires touching code with no existing tests. Where do you draw the line on test scope?Test coverageSenior–staff+

Using floating-point for price comparisonA PR uses floating-point math to compare prices. The author argues the values are always under a million so precision doesn't matter. How do you respond?CorrectnessMid–senior

Generic exceptions with message parsingA PR throws a generic exception in three different failure modes. Each call site has to inspect the message to decide what happened. How would you push back?Error handlingMid–senior

Unnecessary module rewrite by juniorA junior engineer's PR rewrites a working module they didn't need to touch. The rewrite is fine but unnecessary. What's your comment?Feedback deliverySenior–staff+

Indefinite API response cachingA PR caches API responses indefinitely. The author says the responses don't change often. What do you ask?CorrectnessMid–senior

Parsing JSON without size limitsA PR parses user-supplied JSON without specifying limits. What concerns would you raise?Edge casesMid–senior

Abstraction that breaks existing patternsA PR introduces a new abstraction that doesn't quite fit any of the existing patterns. The code works. What do you do?ReadabilitySenior–staff+

Using Thread.sleep in async testsA PR's test relies on Thread.sleep to wait for an async result. What concerns would you raise?Test coverageMid–senior

Adding multiple boolean parametersA PR adds three new boolean parameters to an existing function: 'force', 'silent', and 'strict'. What's your feedback?NamingMid–senior

Deduplicating objects by referenceA PR uses a Set to deduplicate user objects in a list. The Set treats objects as distinct unless they're identical references. What do you raise?CorrectnessEntry–mid

Blocking after two weeks workYou'd block a PR for a real concern, but the author has worked on it for two weeks. How do you write the comment?Feedback deliverySenior–staff+

Loop calling external APIA PR processes a list of orders one by one in a loop, calling an external API per iteration. What's your concern?Edge casesMid–senior

Fallback on database failureA PR introduces a 'fallback to default' on a database read failure. Walk me through whether that's the right call.Error handlingSenior–staff+

Shared mutable map writesA PR writes to a shared mutable map in a multi-threaded path. The author says reads are 'mostly fine'. What do you say?CorrectnessMid–senior

Tests only checking return typeA PR adds a new utility but tests only check that it returns the right type. How do you respond?Test coverageEntry–mid

Recommending refactor outside scopeYou'd recommend a deeper refactor than this PR is scoped for. The work is real but tangential. How do you make the suggestion without blocking?Feedback deliverySenior–staff+

Vague function name 'handleA PR uses the name 'handle' for a function that loads, transforms, and saves data. What do you suggest?NamingEntry–mid

Unbounded activity listA PR adds a feature that returns a 'recent activity' list. There's no maximum size. What's your concern?Edge casesMid–senior

Function doing too many thingsA PR's logic is correct but the function does five things. How do you push for separation without dictating the exact decomposition?ReadabilitySenior–staff+

Feature flag scattered across layersA PR adds a feature behind a flag. The flag check happens inside three different layers of the code. Walk me through your concerns.CorrectnessSenior–staff+

Average calculation on empty listA PR adds a function that calculates the average of a list of numbers by summing them and dividing by the length. The function returns 0 when the list is empty. What would you point out?CorrectnessEntry–mid

Password reset without email validationA PR adds a function that takes a user's email and sends a password reset link. The function doesn't validate the email format before sending. What edge cases would you ask about?Edge casesEntry–mid

Cryptic variable names in logicA PR adds a function with variables named 'd', 'tmp', and 'res'. The logic is correct but hard to follow. How would you phrase feedback to make it constructive?Feedback deliveryEntry–mid

Date range filter validationA PR adds a new endpoint that accepts a date range filter. The tests verify it works with valid dates but don't check what happens when start_date is after end_date. What's your feedback?Test coverageEntry–mid

Overly generic function name 'processA PR adds a function called 'process' that reads a file, parses JSON, validates fields, and writes to a database. What naming concern would you raise?NamingEntry–mid

Missing config fileA PR adds code that reads from a configuration file but doesn't handle the case where the file doesn't exist. Walk me through what you'd check for.Error handlingEntry–mid

In-memory search on large datasetA PR adds a search function that loops through all records and filters in memory. It works fine in the test database with 50 records. What would you ask about?Edge casesEntry–mid

AI-generated code reviewA teammate opens a 600-line PR and mentions most of it came from an AI coding assistant. The code looks plausible and the tests pass. How does that change your review, if at all?Ai Generated codeMid–staff+

Auto-generated tests with wrong assertionsA PR's tests were auto-generated, and each one asserts whatever the function currently returns — including one case whose expected value looks wrong. What's your review comment?Test coverageMid–senior

API key in config fileYou spot what looks like a live API key committed in a PR's config file. The author says it's only for testing and they'll remove it before merge. Walk me through your response.Security awarenessEntry–mid

SQL injection with frontend validationA PR builds a SQL query by concatenating a user-supplied search string into the WHERE clause. The author points out the input is validated on the frontend. What do you say?Security awarenessEntry–mid

Adding a third-party dependencyA PR adds a new third-party package to save roughly twenty lines of code. How do you decide whether that trade is worth it?Dependency hygieneMid–senior

Database migration with code couplingA PR ships a database migration that renames a column together with the code change that uses the new name. What's the deployment problem hiding here, and what do you ask for?CorrectnessSenior–staff+

Coupon count check and decrementA PR checks a coupon's remaining-use count with one query, then decrements it with a second write if it's positive. Two reviewers already approved. What do you flag?CorrectnessMid–senior

Logging sensitive request dataTo debug a flaky integration, a PR adds logging of full request and response bodies on every call. What would you push on before approving?Security awarenessMid–senior

Large refactor with behavior changesYou're assigned a 3,000-line PR that mixes a mechanical refactor with real behavior changes. Line-by-line review isn't realistic. What do you actually do?Review processSenior–staff+

Skipping tests to fix CIA PR fixes a real bug but also marks two flaky tests as skipped to get CI green. How do you respond?Test coverageMid–senior

LLM prompt injection riskYou're reviewing code that builds an LLM prompt by inserting user-supplied text directly into the system instructions, then acts on the model's reply. What risks do you raise?Security awarenessSenior–staff+

Timeout longer than SLAA PR sets a 30-second timeout on an upstream call that runs inside a request handler with a 10-second SLA. Nobody else flagged it. What's your comment?Error handlingMid–senior

Disagreeing with reviewer feedbackYou're the PR author this time. A reviewer leaves fifteen comments and you disagree with several. How do you decide what to push back on, and how do you do it?Feedback deliveryEntry–mid

Magic number duplicationA PR hardcodes the number 86400 in four different places. The logic is correct. What do you suggest, and why does it matter beyond style?ReadabilityEntry–mid

SQL injection via template literalsA PR adds a new function that concatenates user-supplied strings into a SQL query using template literals. The tests pass. What security concerns do you raise?Security awarenessEntry–senior

Transitive dependency version mismatchA PR upgrades a transitive dependency from 2.1.4 to 2.1.5 to fix a low-severity CVE. The direct dependency we use is still pinned to 2.1.3. What do you check before approving?Dependency hygieneMid–staff+

Messy commit historyA junior engineer's PR has 15 commits with messages like 'fix', 'wip', and 'actually fix it now'. The code itself is good. Do you ask them to squash? How do you explain why?Review processMid–senior

Stale permission cacheA PR introduces a caching layer that stores user permissions in Redis with a 1-hour TTL. Walk me through the edge cases around stale data and revoked access.Edge casesMid–staff+

Pagination defaulting to all recordsA PR adds pagination to an endpoint but the new page_token parameter is optional and defaults to fetching all records. What feedback do you give about API design?CorrectnessMid–senior

Suspected AI-generated codeA PR contains code that looks suspiciously like it was AI-generated: variable names are generic, there are unused imports, and the structure mimics a textbook example. How do you approach this?Ai Generated codeMid–staff+

Tests mocking refactored internalsA PR refactors a critical payment processing function but doesn't update the existing unit tests—they still pass by mocking the same internal methods. What's your concern?Test coverageSenior–staff+

Script tag XSS prevention gapsA PR adds input validation that rejects any string containing '<script'. The author says this prevents XSS. What additional concerns or gaps do you raise?Security awarenessEntry–mid

Undefined arguments in signature changeA PR changes a function signature from processData(data) to processData(data, options = {}) for backward compatibility. Three calling sites pass undefined as the second argument. What do you say?ReadabilityEntry–mid

Catch-all error handling with re-throwA PR adds error handling that catches all exceptions, logs them to Sentry, then re-throws. The author argues this gives better observability. What trade-offs do you discuss?Error handlingMid–senior

Duplicate functionality in new serviceA PR introduces a new microservice dependency that duplicates functionality already provided by an existing internal service. The new one has better docs. How do you frame this concern?Dependency hygieneSenior–leadership

Complex algorithm without commentsA staff engineer's PR implements a complex algorithm without comments. When asked, they say the code is self-documenting. You disagree. How do you push back?Feedback deliverySenior–staff+

Memory scaling in batch processingA PR adds a batch processing job that loads all user records into memory, transforms them, then writes back. It works fine in staging with 10K users. What do you flag?Edge casesEntry–mid

Inconsistent boolean function namingA PR renames isValid to checkValid but the function still returns a boolean. Three teammates have already approved. Do you block on this naming inconsistency?NamingEntry–senior

Feature flag in tight loopA PR adds a feature flag check inside a tight loop that processes millions of records. The flag is evaluated fresh on every iteration. What performance concern do you raise?CorrectnessMid–senior

Unpinned base image versionA PR updates our Dockerfile to use latest instead of a pinned base image version. The author says this ensures we always get security patches. How do you respond?Dependency hygieneMid–senior

Integration tests without unit testsA PR adds comprehensive integration tests but no unit tests. The author argues integration tests provide better coverage. What's your take on the testing strategy?Test coverageMid–senior

Hardcoded AWS regionA PR hardcodes an AWS region as us-east-1 in a new service. The rest of the codebase reads it from environment variables. Do you ask them to change it?CorrectnessEntry–mid

Logging PII to third-party servicesA PR adds logging that includes the full request payload, which may contain PII. The logs go to a third-party service. Walk me through your concerns.Security awarenessMid–staff+

In-memory rate limiter across instancesA PR implements a rate limiter using an in-memory counter. It works in development. What questions do you ask about production deployment with multiple instances?Edge casesMid–senior

Evaluating unfamiliar third-party librariesA PR from a contractor includes a new third-party library you've never heard of. It has 47 GitHub stars and was last updated 8 months ago. How do you evaluate whether to approve?Dependency hygieneSenior–staff+

TODO comments before production releaseA PR adds a TODO comment saying 'refactor this later' in code that's about to be released to production. The code works. Do you require them to address it now or let it go?Review processEntry–senior

Code duplication instead of extractionA PR copies 80 lines of code from another file and modifies 3 lines instead of extracting a shared function. The author says they might diverge later. What do you recommend?ReadabilityEntry–mid

Removing error handlingA PR deletes error handling around a third-party API call because 'the service has 99.9% uptime so we don't need it'. How do you frame your feedback?Error handlingEntry–mid

Reviewing AI-generated codeA PR includes AI-generated code with a comment block that says 'Generated by GPT-4' and a detailed prompt. The code works and has tests. What's your review stance?Ai Generated codeMid–staff+