A code review is a check by another engineer before a change merges. It catches bugs, spreads knowledge of the codebase and keeps the design consistent. As a new grad you'll be asked to review sooner than you expect, and your reviews count. You don't need to be an expert in the code to spot unclear logic or a missing test.
The goal of a review#
A review isn't a hunt for perfect code. The useful standard is this: approve a change when it clearly makes the codebase better, even if it isn't how you'd write it.
Two things follow from that standard. First, block only for real problems, not for personal taste. Second, don't let a change that makes things worse pass only to be polite.
What to look for, in priority order#
With that standard in mind, spend your attention where bugs cost the most. Work from the top of this list down, and stop to comment when you find a big problem.
| Priority | Area | Questions to ask |
|---|---|---|
| 1 | Correctness | Does it do what the description says? Edge cases, errors, nulls, concurrency? |
| 2 | Design | Does it belong here? Is it simpler than it could be? Will it be easy to change? |
| 3 | Tests | Do tests cover the new behavior and the failure cases? Would they fail if the code broke? |
| 4 | Security | Is input validated? Any secrets in code? Any injection risk or missing permission check? |
| 5 | Readability | Are names clear? Can a new reader follow the logic? Are comments about why? |
Style isn't on the list on purpose. Leave style (formatting, import order, quote marks) to automated tools such as formatters and linters. If a style rule matters, add it to the linter config instead of a review comment. That way, no human spends time on it again.
Start with the description and the tests#
Knowing what to check is one thing; knowing where to start reading is another. Read the PR description before the code. If you don't know what the change should do, you can't judge whether it does it. Ask for a better description if it is missing.
Then read the tests before the implementation. The tests show the intended behavior and the cases the author thought about. Missing cases are often the first bug you find. After that, read the main logic, and leave generated or mechanical files for last.
How to write comments#
Once you find something, how you phrase it decides whether it gets fixed. A good comment says what the problem is, why it matters and what would fix it. Compare these comments on the same line.
| Weak | Better |
|---|---|
| "This is wrong." | "This returns None when the list is empty, so total() fails on line 40. Return 0 here instead?" |
| "Rename this." | "data hides what this holds. Something like pending_orders would help the next reader." |
| "Why not use a set?" | "Lookups here are inside a loop. A set would make each one instead of ." |
The better versions share a few habits:
- Explain why. A reason teaches the author. An order without a reason only gets obeyed.
- Ask when unsure. "What happens if the request times out here?" invites an answer, and you may learn something.
- Comment on the code, not the person. Write "this function" instead of "you".
- Be specific. Point to the line, and suggest a fix or an example when you can.
- Say what's good. A short "Nice, this is much clearer" makes the useful feedback easier to hear.
Label how serious each comment is#
Even a well-written comment leaves one question open: does the author have to fix it before merge? Prefix each comment with a label so nobody has to guess.
| Label | Meaning |
|---|---|
| Blocking | Must be fixed before merge (bug, security issue, broken design) |
| Nit | Small polish; the author may ignore it |
| Optional or Consider | A suggestion worth thinking about, not required |
| Question | You need an answer to continue the review |
| FYI | Information for the future; no action needed |
If most of your comments are nits, the PR is probably in good shape. Approve it and let the author decide on the nits.
Approve, comment or request changes#
When you've gone through the whole change, you submit the review. Most review tools give you three choices. Pick the one that matches what the author must do next.
| Choice | Use it when |
|---|---|
| Approve | The change is ready to merge, or only nits remain |
| Comment | You have questions or ideas but no verdict yet |
| Request changes | At least one blocking problem must be fixed first |
You can approve with comments, for example "Approved, with two small nits". This trusts the author to fix them without another round. On many teams, "request changes" blocks the merge until you approve, so re-review quickly once the author pushes fixes.
Review quickly#
A slow review blocks your teammate and slows the whole team. Google's guidance is to respond within one business day at the most.
- Review when you finish a task or come back from a break, not in the middle of focused work.
- If you can't do a full review soon, say so and give a time.
- For a very large PR, ask the author to split it before you review it line by line.
- If you only understand part of the change, review that part and say which part you skipped.
Fast doesn't mean careless. A quick first response, even a partial one, keeps the author moving.
Key takeaways#
- Approve when the change makes the code better, even if it is not perfect.
- Check correctness, design, tests, security and readability, in that order.
- Leave style to linters and formatters.
- Read the description and the tests before the implementation.
- Explain why, ask questions and label each comment as blocking, nit or optional.
- Respond within a business day, and be kind and specific.