Code Review Philosophy and Practice Questions

Covers the approach to conducting effective code reviews, including what reviewers look for and how reviewers provide constructive feedback. Topics include evaluating correctness, design and architecture, complexity, test coverage and quality, performance, security considerations, readability and maintainability, and consistency with style and team conventions. Includes techniques for balancing thoroughness and development velocity, using checklists and automation to reduce repetitive comments, unblocking reviewees, preserving morale and psychological safety, resolving disagreements, and using code reviews as opportunities for mentoring and knowledge transfer. Candidates may also discuss tooling, review workflow, time boxing, and metrics for measuring review effectiveness such as review turnaround time and post review defect rates.

EasyBehavioral

57 practiced

As a reviewer, how do you provide constructive feedback that preserves morale and psychological safety? Describe at least six concrete practices (phrasing, prioritization, praise, examples, alternatives, next steps) and explain why each helps the author receive and act on the feedback.

HardBehavioral

59 practiced

A reviewer used harsh language in comments and the author felt publicly humiliated. As the engineering manager, describe a stepwise plan to de-escalate the situation, repair relationships, update code review guidelines, and prevent similar incidents, including any coaching, documentation changes, and follow-up measurements.

Sample Answer

Situation: In a recent code review a senior reviewer used harsh language; the author felt publicly humiliated and the team morale dipped.

Task: As the engineering manager, I needed to de‑escalate immediately, repair trust between the individuals, update our code review process to prevent recurrence, and measure that psychological safety improved.

Action:1. Immediate de‑escalation (first 24 hours)- Privately message the author to acknowledge and offer a safe space to talk; avoid public discussion.- Privately speak with the reviewer to understand intent and ask them to refrain from public comments while we sort this out.- If needed, mediate a short private meeting with both parties (neutral tone, set ground rules) to let the author explain impact and the reviewer listen without defending.

2. Repair relationships (days 1–7)- Facilitate a mediated conversation where the reviewer can apologize and both agree concrete steps (e.g., reviewer rephrases comments publicly to correct tone).- Pair each with a trusted peer for short-term buddy support on the next 2–3 reviews to rebuild trust.

3. Coaching and accountability (week 1–4)- One‑on‑one coaching with the reviewer focused on feedback delivery (concrete examples, “I” statements, actionable suggestions), using role‑play.- Offer the author support (mentor check‑ins) and coaching on responding to harsh feedback constructively where appropriate.- If behavior repeats or is severe, follow progressive discipline consistent with HR policy.

4. Process & documentation changes (week 1–3)- Update Code Review Guidelines: add explicit sections on respectful language, examples of constructive vs. harsh comments, and a 3‑step template (summary, rationale, suggested change).- Add a required PR description template and a reviewer checklist that includes “tone check” and “is feedback actionable?”.- Enable a brief pre‑merge “review sanity” step where a peer quickly scans major comments for tone on high‑impact PRs.

5. Tooling & automation (ongoing)- Introduce a lightweight comment-lint rule (bots that flag profanity or aggressive phrasing) with gentle suggestions rather than blocking.- Add a short “reaction” policy: if an author flags a comment as harmful, notify the manager for follow-up.

6. Preventive training & onboarding (monthly / ongoing)- Run a 60‑minute workshop on psychological safety and giving/receiving feedback; include live exercises.- Add Code Review Etiquette to onboarding and career development plans.

7. Measurements & follow‑up (30–90 days)- Track metrics: number of “tone flags” on PRs, anonymous team psychological‑safety pulse (e.g., 5‑question survey), code review turnaround time, and number of escalations.- Review metrics monthly; aim for zero repeated incidents and a measurable lift in pulse scores (target: +10% within 3 months).- Share aggregated outcomes and guideline updates in a team retro to keep transparency.

Result / Learning: This approach resolves the immediate harm, holds the reviewer accountable while coaching growth, and embeds systemic changes so feedback stays constructive. I learned that combining private repair, clear documented expectations, light automation, and measurable follow‑up restores trust and prevents recurrence.

MediumTechnical

54 practiced

Design a custom lint rule (no implementation required) that enforces usage of a secure random function for token generation instead of non-cryptographic RNGs in a JavaScript codebase. Specify the detection heuristic, examples of violations and allowed patterns, false-positive risks, and minimal unit tests you would write for the rule.

Sample Answer

Detection heuristic:- Static AST-based rule scanning CallExpression and MemberExpression nodes.- Flag direct uses of known non-cryptographic RNG APIs: - MemberExpression: Math.random() - Calls to common non-crypto libraries: require('seedrandom')(...), require('random-js') factory/random(), _.random(), faker.random.* etc. - Identifiers named random() where the definition resolves to a known non-crypto source (simple local resolution).- Allow only explicit crypto-secure APIs: - Node: require('crypto').randomBytes(), crypto.webcrypto.getRandomValues(), crypto.getRandomValues (if Node supports), crypto.randomInt (Node >=14.10) - Browser: window.crypto.getRandomValues(), crypto.subtle (where appropriate)- Optionally perform limited intra-file flow analysis: if token generation function calls a secure RNG, mark as allowed.

Examples of violations:- const token = Math.random().toString(36).slice(2);- const rng = require('seedrandom')(); const t = rng().toString();- _.random(0, Number.MAX_SAFE_INTEGER).toString(36)

Allowed patterns:- const buf = crypto.randomBytes(16).toString('hex');- const arr = crypto.getRandomValues(new Uint8Array(16));- // wrapper that delegates to crypto.randomBytes function secureToken(n){ return crypto.randomBytes(n).toString('hex'); }

False-positive risks and mitigations:- False positive: user-defined function named random that internally uses crypto. Mitigation: resolve identifier to its local definition and check if that def calls a secure API.- False positive: test mocks or polyfills where Math.random is intentionally used; mitigate by allowing rule-disable comments (// eslint-disable-next-line secure-rng) and by recognizing files under test/ or *.spec.js.- False negative: obscure wrappers or third-party libs that do secure RNG but not known by rule; mitigate by providing a configurable allowlist of module names or exported function signatures.

Minimal unit tests (each asserts violation or allowed):1) Violation — Math.random used: "const t = Math.random();"2) Violation — seedrandom require: "const seedrandom = require('seedrandom'); const t = seedrandom()();"3) Allowed — crypto.randomBytes: "const { randomBytes } = require('crypto'); const t = randomBytes(16).toString('hex');"4) Allowed — window.crypto in browser: "const a = window.crypto.getRandomValues(new Uint8Array(8));"5) False-positive mitigation — local wrapper that uses crypto (should be allowed): "function r(){ return require('crypto').randomBytes(8); } const t = r();"6) Test-file exemption — Math.random in test file should be allowed when rule is configured to ignore tests: "// file: foo.test.js\nconst t = Math.random();"

Each test should assert the rule reports (or not) at the CallExpression location, include message text pointing to using secure RNG, and cover autofix suggestion text (replace pattern) where safe: recommend using crypto.getRandomValues/randomBytes.

EasyTechnical

71 practiced

Describe what makes a good commit message and a good commit granularity for PRs. Provide two concrete examples of 'bad' commit histories and transform each into an improved commit strategy that helps reviewers and future maintainers.

Sample Answer

A good commit message and appropriate commit granularity make reviews faster, history easier to understand, and bisects/reverts safer.

Good commit message:- Short subject line (50 chars) summarizing intent, present tense (e.g., "Fix null pointer in user lookup")- Blank line, then a short body (wrap ~72 chars) explaining why, not just what; mention side effects, related ticket/PR, and testing performed.- Use conventional prefixes if your team uses them (feat/, fix/, chore/).

Good commit granularity:- One logical change per commit: keep commits small enough to review (~1–3 focused files) but complete (compiles/tests pass).- Prefer feature-branch squashes for noisy WIP commits, but preserve meaningful intermediate commits (API changes, migrations).

Example 1 — Bad history:- Commit 1: "WIP"- Commit 2: "more changes"- Commit 3: "fixed bug"Why bad: vague messages, mixes refactoring, feature, and bugfix; hard to review or revert.

Improved strategy:- During development keep WIP locally; before PR, rebase/squash into 3 commits: 1) "Add user profile API endpoint (RFC-123)" 2) "Refactor user model: extract validation logic" 3) "Fix NPE in user lookup when email missing — add test"Each commit has clear scope and tests, making review and future bisecting straightforward.

Example 2 — Bad history:- One huge commit: "Implement user dashboard" changing 200 files, touching styling, backend, tests, and config.Why bad: too large to review, mixes concerns, risky to revert.

Improved strategy:- Split into focused commits/PRs or stacked PRs: 1) "Add dashboard backend: controllers and endpoints" (includes API + unit tests) 2) "Add dashboard frontend components" (UI + integration tests) 3) "Add responsive styling for dashboard" (CSS only) 4) "Update CI to run dashboard integration tests"This separation isolates reviewable units and makes rollback targeted.

Takeaway: write intentful messages, keep commits logically atomic, and squash WIP noise before merging so history tells a clear, actionable story.

MediumTechnical

66 practiced

A PR contains 120 small 'WIP' commits and non-descriptive commit messages. As a reviewer, describe your approach to make the review efficient: propose whether to ask the author to squash, draft a suggested squashed commit message, and explain how to preserve important history while improving reviewability.

Sample Answer

Approach (goal: make reviewable while preserving meaningful history)- Ask the author to collapse the 120 WIP commits into a small set of logical commits (1–6) that each represent a single coherent change (e.g., "add X feature", "refactor Y", "fix concurrency bug"). Explain that this improves reviewability and git history while keeping important milestones.

Practical steps I’d request / perform1. Create a backup branch so nothing is lost:

bash

git checkout -b feature/branch-backup

2. Interactive rebase to fold WIP commits into logical commits (author does this):

bash

git checkout feature/branch
git rebase -i origin/main   # mark WIP commits as squash/fixup into logical commits
# or use: git rebase -i --autosquash <base>

3. Use "fixup!" for trivial fixes and "squash!" for cases needing combined messages. After rebase, force-push:

bash

git push --force-with-lease origin feature/branch

Suggested squashed commit message (example)Title: Implement X feature; refactor Y; fix ZBody:- Implement core X functionality (adds X service, endpoint /x)- Refactor Y to extract shared utilities (files: a.py, b.py)- Fix race condition in Z by adding mutex in z_handler- Tests: add unit tests for X, add integration test for Z- Notes: follow-up cleanup planned in separate PR (issue #123)

Preserving important history- Keep any semantically meaningful commits separate (e.g., migration, API contract changes) instead of squashing them away.- If some intermediate WIP commits contain useful debugging info, move that info into the combined commit message or use git notes to preserve raw snapshots:

bash

git notes add -m "debug logs: ... " <commit>

- Encourage the author to include references to issue/PR numbers and short rationale in commit messages so intent survives squashing.

Review workflow alternatives if author can’t rewrite- Use a local temporary branch to create a clean squashed branch for review only:

bash

git checkout -b review-squash origin/feature/branch
git reset $(git merge-base review-squash origin/main)
git add -A
git commit -m "Squashed review commit: ... (see original branch for raw commits)"

- Use GitHub/GitLab features: "Changes from last review", file filters, or diffing tools (git range-diff) to compare logical chunks.

Why this approach- Reduces cognitive load for reviewers (small, focused commits)- Preserves intent via good messages and separate commits for distinct concerns- Keeps original work recoverable via backup branch or git notes- Uses standard git workflow (interactive rebase + force-with-lease) and provides fallback for teams that forbid history rewrites

Edge notes- Always ask permission before force-pushing shared branches.- Suggest a short contribution guideline: "squash WIP locally; keep meaningful commits; write one-line title + 3–5 bullet message."

Unlock Full Question Bank

Get access to hundreds of Code Review Philosophy and Practice interview questions and detailed answers.

Join thousands of developers preparing for their dream job.