Craft of code
Code review
Author small, reviewable PRs and give reviews that improve both the code and the author — design first, tone that teaches, turnaround that unblocks.
Code review is how a team keeps code health improving one change at a time. Google's published standard is blunt: approve a change when it improves the codebase, even if it is not perfect. Blocking for perfection creates huge PRs, stale branches and demoralized authors. Rubber-stamping creates slow-motion rot.
You wear two hats: author (make the change easy to review) and reviewer (protect correctness and teach without drowning the author in nits). SDE2s are expected to be strong at both.
Authors: aim for under ~400 lines of meaningful diff, describe what/why/test/rollout, and self-review first. Reviewers: design, then correctness, then tests, then names, then nits. Configure CI so humans never argue braces. LGTM means you would own the change on main — it is not a glance.
Concepts in this section
Read them in order.
| Step | Concept | What it answers |
|---|---|---|
| 1 | Reviewable PRs | How small, described and stacked should a change be? |
| 2 | What to look at | Design first — what does LGTM mean? |
| 3 | Review tone | How do you phrase a catch so it teaches? |
| 4 | Turnaround | How fast is "fast enough," and what if you cannot review now? |
| 5 | Block vs approve | When do you hold the merge, and when do you follow up? |
| 6 | Ops and security | Authz, secrets, migrations, flags, load. |
A split that saves calendar time
A 1,200-line "billing v2" PR gets skimmed. Split it: schema migration + expand-contract, domain logic + tests, API handlers, flag default-off. Same feature, four reviewable slices, each mergeable. Total calendar time usually drops.
LGTM means you would own the change on main. If you do not understand it, you are signing a blank check.
Slow review is a delivery bug: first response within a business day, a redirect if you cannot look now, never silence. Block what is wrong, unsafe or expensive to undo (migrations, public APIs, authz). Approve improvements and track leftover polish. After two circling rounds, huddle, then post the decision on the PR.
Common mistakes
- Megadiffs and missing what/why/test/rollout in the description.
- Nits before design. Order is design → correctness → tests → readability → nits.
- Fake full LGTM after a skim of risky files. Partial review, named gaps.
- Silence. Redirect to another reviewer if you cannot look today.
- Blocking on taste or approving an unsafe migration to "unblock."
Key takeaways
- Review for continuous improvement of code health, not perfection.
- Authors: small PRs, crisp why/what/how-tested, self-review first.
- Reviewers: design then correctness; questions and labeled nits; fast honest turnaround.
- Block what is unsafe or expensive to undo; approve improvements with tracked polish.
- Ask who can call this and what happens at 10× traffic.
Further learning
Google Engineering Practices: Code ReviewGoogle’s Engineering Practices documentationeng-practices What to look for in a code reviewGoogle’s Engineering Practices documentationeng-practices Conventional CommentsComments that are easy to grok and grepConventional Comments
Software Engineering at Google — Code Review chapterSoftware Engineering at Googleabseil.ioRelated
Writing code others can maintain
Managing complexity with deep modules, clear naming, honest comments, consistent style, and error strategies that shrink the blast radius of change.
Git internals and workflow
Git as a content-addressed object store plus a graph of commits and movable pointers, so merges, rebases, bisects and recoveries stop feeling risky, along with the team workflows that go with them.
Collaboration and disagreement
Build trust before you need it, disagree with data and options, and make cross-team asks easy to grant.