Code Quality and Review Questions

Evaluating code for correctness, clarity, maintainability, performance, and security and providing actionable feedback. Candidates should be able to identify bugs, logical errors, anti patterns, and algorithmic inefficiencies, and to suggest concrete fixes or refactorings. Reviews should cover naming, modularity, documentation, testability, and design patterns, and recommend appropriate unit and integration tests. Candidates should also diagnose performance issues, reason about trade offs when optimizing, and consider how changes affect reliability and security.

EasyBehavioral

70 practiced

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).

EasyTechnical

82 practiced

You open a PR that contains 25 files with mixed issues: a bug in a provisioning script, a security misconfiguration in Terraform, and many minor style issues. As the reviewer, explain how you would triage and classify comments into 'must-fix before merge', 'should-fix before merge', and 'optional', and give two example comments for each category with justification.

EasyTechnical

98 practiced

As a systems engineer reviewing infrastructure code (Terraform, Ansible, Bash, Python), create a practical code-review checklist you would apply to pull requests. The checklist should cover correctness, clarity, maintainability, performance, security, testability, operational readiness (observability, rollback), documentation, and dependency/secret handling. For each checklist item include a 1-2 sentence rationale and a concrete example of what to look for in PR diffs or code.

MediumSystem Design

99 practiced

Design a code-review checklist specifically for Terraform modules in a large, multi-team organization. The checklist should cover module interface design (inputs/outputs), naming/tags, use of locals, avoidance of provider-specific hardcoding, state backend considerations, reuse, documentation/examples, and security controls. Provide the checklist and explain why each item matters at organizational scale.

Sample Answer

**Clarify scope**- Checklist for reusable Terraform modules used across many teams; focuses on interface, naming, state, reuse, docs, and security.

**Module Interface (inputs / outputs)**- Validate minimal, opinionated inputs with sensible defaults; inputs documented and typed; avoid huge maps/lists for simple fields.- Ensure outputs are stable, named with module prefix and only export necessary values.Why: Small, typed interfaces reduce integration friction and accidental breaking changes across teams.

**Naming & Tags**- Enforce resource/name conventions and required tags (owner, environment, cost-center, terraform-module).Why: Consistent naming enables cross-team discovery, cost allocation, and automation.

**Use of locals**- Check locals for computed values; avoid duplicating logic in resources; ensure locals do not hold secrets.Why: Locals improve readability and single-source-of-truth for derived values.

**Avoid provider-specific hardcoding**- No hardcoded ARNs, regions, account IDs, or provider configurations; use provider aliases and inputs.Why: Keeps modules portable across accounts/regions and supports multi-environment deployment.

**State backend considerations**- Module must be backend-agnostic; include guidance for locking and workspace design; do not configure remote backend in module.Why: Prevents state conflicts and enforces organizational backend standards.

**Reuse & Composition**- Prefer smaller modules that compose; avoid huge “kitchen-sink” modules; provide extension points.Why: Composable modules improve testability and reduce duplication.

**Documentation & Examples**- Require README with input/output tables, example usage, lifecycle notes, and upgrade/migration instructions.Why: Lowers onboarding friction and prevents misuse.

**Security Controls**- Scan for secrets, enforce least privilege IAM, validate encryption settings and public access flags.Why: Centralizes policy enforcement and reduces blast radius.

**Quality gates**- Require unit tests (terraform validate/plan), CI checks, changelog, and semantic versioning.Why: Ensures safe rollouts and backward compatibility for many consumers.

MediumTechnical

68 practiced

You are reviewing an Ansible playbook intended to be idempotent. Identify problems in this snippet and propose changes to make it idempotent and testable.

yaml

- hosts: web
  tasks:
    - name: install nginx
      command: apt-get install -y nginx
    - name: create conf
      copy:
        content: "server { listen 80; }"
        dest: /etc/nginx/sites-enabled/default
    - name: restart nginx
      service:
        name: nginx
        state: restarted

What would you change and why? How would you test the playbook in CI?

Sample Answer

**Brief summary of problems**- Uses raw command apt-get: not idempotent, no package facts, no `update_cache`.- `copy` with fixed content will always report changed unless content equals existing file; ok but better to manage template or notify handler.- `service: state: restarted` always restarts service (not idempotent).

**Proposed changes**- Use Ansible modules (apt, template/copy with checksum) and handlers for restart.- Ensure package cache handled and update only when needed.- Use `notify` to restart only when configuration changed.

Example corrected playbook:

yaml

- hosts: web
  become: true
  tasks:
    - name: ensure apt cache is updated
      apt:
        update_cache: yes
      changed_when: false

    - name: install nginx
      apt:
        name: nginx
        state: present

    - name: place nginx site config
      copy:
        content: "server { listen 80; }"
        dest: /etc/nginx/sites-available/default
        owner: root
        group: root
        mode: '0644'
      notify: reload nginx

  handlers:
    - name: reload nginx
      service:
        name: nginx
        state: reloaded

**Why these changes**- `apt` is idempotent and reports accurate changed state. `copy` compares checksums so only changes when content differs. Using handler prevents unnecessary restarts; `reloaded` is less disruptive than `restarted`.

**How I'd test in CI**- Use Molecule with Docker or LXD driver to converge the role.- Tests: - Molecule converge runs twice; verify second run reports 0 changed. - Use Molecule verify with Testinfra/pytest to assert package installed, file content, service running. - Add an idempotence step: run `ansible-playbook` twice and fail if second run returns changed > 0.- Integrate into CI (GitHub Actions/GitLab CI): spin up ephemeral instance, run `molecule test` or run playbook + idempotence check + Testinfra tests.

Unlock Full Question Bank

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

Join thousands of developers preparing for their dream job.