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