Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Notes for Google's Code Review best practice doc https://google.github.io/eng-practices/review/reviewer/

How to do a code review

Notes from Google’s Engineering Practices documentation

Terms

  • CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.”
  • LGTM: Means “Looks Good to Me.” It is what a code reviewer says when approving a CL.
  • emergency: small change allows a major launch to continue instead of rolling back, fixes a bug significantly affecting users in production, handles a pressing legal issue, closes a major security hole

Standard

  • Purpose - make sure the overall code health of Google’s code base is improving over time
  • In General - reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on
  • There is no such thing as perfect code only better code
  • Rather than profection, a reviewer should seek continuous improvement
  • Prefix non-critical comments with “Nit: ”
  • Never worsen a codebase unless it’s an emergency

Mentoring

Sharing knowledge is part of improving the code health of a system over time.

  • Make sure it’s clear that this is for education and not critical for merging the changelist

Principals

  • Technical fact over preferences
  • Styleguide or it’s personal preference
  • If an author can demonstrate that there are several valid approaches, CR should defer to author
  • author may ask for consistency if that doesn’t worsen overall codehealth

Conflicts

  • resolve conflict using guidelines
  • Have a meeting, but record results of the meeting on the patchset
  • escalate to team lead or eng manager, don’t let a CL sit around due to conflict

What to look for in a code review

Design

  • Does this “make sense”?
  • Is this where this code belongs?
  • Is now the time to make this change?
  • Does this integrate well with the rest of the system

Functionality

Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review.

…disagree

  • Does what the dev intended
  • We want what the dev intended
  • UI changes in particular
  • parallel programming: think really hard about it
  • regexes, too, I reckon

Complexity

  • too complex can’t be understood quickly by code readers/developers likely to introduce bugs when modifying this
  • over-engineering - solving future problems that we don’t know about yet

Tests

  • Add tests with CL that adds functionality
  • Tests must “be sensible”
  • Break when code breaks, work only when code works
  • Don’t accept complexity

Naming

  • “good names”
  • Long enough to describe what it does without being too long to read

Comments

  • Understandable
  • Explain why code exists not what code is doing
  • Maybe this change resolves or conflicts with another comment

Style

Don’t block CLs from being submitted based only on personal style preferences.

  • Don’t make style changes with other changes

Documentation

  • Check to see if changes how code is build, tested, released, interacted with
  • Deprecated code? remove/indicate in documentation

Every line

  • Don’t skip over any part of the code review
  • Make sure you understand what the code does
  • Make sure you tell the reviewer if you don’t understand the code

Context

  • File context
  • System context

Good Things

  • Say something nice
  • More helpful in mentoring terms

Navigating a CL in review

Broad overview

  • Check the description: what + why
  • Does the change make sense?
    • Reject with suggestion of what dev should have done if possible
  • Lots of changes you don’t want?
    • post your dev process more publicly

Main

  • If you can’t find the “main part” ask, or ask to split this up into multiple CLs
  • See if you see “Design problems” in the “main part”
  • Send that first since:
    • Devs could submit work on top of a CL
    • People have deadlines

Detail

  • Read all the files
  • Maybe read tests first

Speed

When code reviews are slow

  • team velocity goes down (cycle time)
  • developers protest the process entirely
  • code health could go down as pressure goes up

How fast?

  • one business day
  • don’t interrupt yourself though
  • time to first response vs time to lgtm: only concerned with the former

LGTM with comments

  • especially across timezones

Large CLs

  • split it into smaller CLs
  • quick comments on design first
  • unblock the developer
  • emergencies and hard deadlines

How to write CR comments

  • Be nice

it is the developer’s responsibility to fix a CL, not the reviewer’s

  • Balancing “fix a CL” w/unblocking
  • Asking a developer to clarify == comment or simplify

Handling Pushback in code reviews

Who is right?

  • sometimes it’s them, tell them and drop it
  • Be polite, demonstrate you hear what they’re saying, but if it’s about code health, insist on it

Upsetting developers

  • Doesn’t happen a lot
  • More about tone than content

Cleaning it up later

  • Doesn’t happen
  • Common way for codebases to degenerate
  • New complexity? Clean it up
  • Exposes surrounding problems? File a task, add a TODO to the CL

General complaints about strictness

  • This is a complaint about speed
  • May take a while for them to see any value in strictness

thcipriani

Meta thoughts

  • LGTM != merge && LGTM != deployment
  • I wonder if +2 == merge holds up CR?

Standard

  • In general favor merging patchsets
  • “Code Health” needs a definition here
  • I like the mentoring/pedegogy part
  • Technical Facts are hard to come by, in my view

What to look for

  • reads like a checklist
  • “Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review.” is a thing I don’t think is true
  • Good Things is undervalued, I think
  • Good points about comments and documentation
  • Another thing I’d add is spelling - weirdly this comes up a lot
  • I think more guidance is needed for “design” – what does “make sense” mean for a CL?

Navigating CL

  • under-emphasis on change description
  • It’s in the developer part of this documentation

Speed

  • Less than 24 hrs to first response, not LGTM

How to write a CR comment

  • “Be nice” heuristic: if you’re writing “obviously” or “clearly”, you’re being an asshole
  • There is a power imbalance here
  • High context vs low context

Pushback

  • Cleanup never happens
  • Most complaints are about CR speed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment