Code Review Process: Principles and Benefits

Mental model

Cisco objective 5.13 says “Describe the principles and benefits of a code review process”. The core idea: no change goes to the main branch without at least one other engineer having read and approved it.

Benefits

1. Catch bugs early

A fresh pair of eyes spots:

• Edge cases the author did not think of.
• A regex that matches too much.
• A missing raise_for_status().
• A hard-coded production hostname.

Fixing a bug in review is minutes; fixing it after it breaks prod is hours plus an incident ticket.

2. Spread knowledge

The reviewer learns the codebase. The author learns from the reviewer’s suggestions. Nobody becomes the single person who knows how X works.

3. Document the “why”

PR descriptions and review conversations become searchable history. Six months later: “why did we switch from Ansible to NSO here?” → git blame → PR → full discussion.

4. Enforce standards

Automated linters catch style and some bugs; a human catches:

• Over-engineering (“this doesn’t need a class”).
• Under-engineering (“you’ll regret hard-coding this”).
• Security issues (“that secret should not be in Git”).

5. Mentorship

Juniors learn by seeing seniors’ reviews. Seniors learn by having their reviews questioned (“why is this better?”).

The mechanics: a PR

1. Branch: engineer creates a feature branch off main.
2. Commit + push: small, focused commits.
3. Open PR: on GitHub / GitLab, “please merge my branch into main”.
4. CI runs: lint, unit tests, build. Pipeline results shown on the PR.
5. Reviewer(s) assigned: by team policy — code owners, round-robin, author’s choice.
6. Review: reviewer reads diff, leaves inline comments or overall comments.
7. Discussion: author responds, pushes updates, requests re-review.
8. Approval: reviewer clicks Approve (or Request Changes).
9. Merge: author (or reviewer) merges when CI is green AND N approvals exist.

What good reviews look like

Good

• “This requests.get has no timeout; it will hang on a stuck server. Add timeout=5.”
• “Nice refactor. The three nested if-statements became one dict lookup.”
• “Can we split this 400-line PR into three smaller ones? Easier to review.”
• Links to prior PRs / docs when relevant.
• Marks nitpicks as such: “nit: typo in comment”.

Bad

• “You should use a different style here” (vague).
• “This is wrong” (not specific; no suggestion).
• “I would have done this differently” (opinion without justification).
• Scope creep: “while you’re at it, also rewrite X” (no, separate PR).
• Silence for days.

PR hygiene

• Small PRs merge faster. 50 lines is a joy; 1000 lines is a chore.
• Clear title + description. What + why + how to test.
• One concern per PR. Mixing “fix a bug” with “refactor the module” buries both.
• Keep the branch rebased or merged with main so reviewers see only your change, not merge noise.
• Respond to all comments before marking resolved — even “ack, will do next PR”.

Review timing

• Target: review within 1 business day.
• Blocking a teammate for 3 days to find the perfect wording is net-negative. Approve with suggestions.
• Pair-program instead when a change needs real discussion.

Common anti-patterns

• Rubber-stamp reviews. “LGTM” on a 2000-line PR with no comments. Not real review.
• Bikeshedding. Three days of debate about naming while bugs ship.
• No style guide. Debating formatting on every PR. Pick a linter, let it be the arbiter.
• One person reviews everything. Bottleneck + they burn out. Rotate.
• No ownership. PRs sit for weeks because no one is assigned.

For network automation specifically

• Review ansible-playbook --check output.
• Review terraform plan output.
• Confirm the dry-run touches only expected devices.
• Watch for hard-coded secrets, IPs, hostnames.
• Verify when: conditions and loops do not accidentally run on all hosts.

FAQ

Does a solo engineer need code review? Not formally. Many solo engineers “review” their own PR 24 hours later with fresh eyes. Catches a surprising number of silly mistakes.

How many approvers? Team policy. Small team: one. Security-sensitive code: two. Critical infra: a code owner + a security owner.

Is pair programming a form of review? Yes, done live. Pair-programmed code can skip a formal review step (the review happened as it was written).

What are “code owners”? A CODEOWNERS file in GitHub / GitLab that automatically assigns reviewers based on which files the PR touches. “Any change under src/network/ → assign @net-team”.