Code Review Flashcards
What should Nit: prefix be used for?
To let the author know that it’s just a point of polish that they could choose to ignore.
Senior principle for approving a PR?
Approve once it’s in a state where it definitely improves the code health, even if the PR isn’t perfect.
3 principles of code review
- Technical facts > opinions and personal preferences.
- Any purely style point that is not in the style guide is a matter of personal preference.
- If several solutions are equally valid, then the reviewer should accept the preference of the author.
Conflict resolution escalation strategy
- Seek consensus in PR comments
- F2F meeting + conclusion notes in PR comments
- Escalate to the team, eng manager
What to look for 1: Overall Design?
- Do the interactions of various pieces of code in the PR make sense?
- Does this change belong in the codebase or in a library?
- Does it integrate well with the rest of the system?
- Is this a good time to add this functionality?
What to look for 2: Functionality?
- Does this PR do what the developer intended?
- Is what the developer intended good for the users of this code?
- Think about edge cases, concurrency problems
- Click through a demo for UI-related PRs
What to look for 3: Complexity?
Is the PR more complex than it should be?
“Too complex” usually means “can’t be understood quickly by code readers.” or “developers are likely to introduce bugs when they try to call or modify this code.”
- Look for overengineering and encourage solving only relevant problems.
What to look for 4: Tests?
- In general, tests should be added in the same PR as the production code.
- Don’t accept complexity in tests just because they aren’t part of the main code.
Helper questions:
- Will the tests actually fail when the code is broken?
- If the code changes beneath them, will they start producing false positives?
- Does each test make simple and useful assertions?
- Are the tests separated appropriately between different test methods?
What to look for 5: Naming?
Did the developer pick good names for everything?
A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read.
What to look for 6: Comments?
Are all of the comments actually necessary and grammarly correct?
Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing.
What to look for 7: Documentation?
If a PR changes how users build, test, interact with, or release code, check README.
What to look for 8: Every line?
- Look at every line of code that you have been assigned to review.
- You should at least be sure that you understand what all the code is doing.
What to look for 9: Context?
- It’s useful to think about the PR in the context of the system as a whole.
- Don’t accept PRs that degrade the code health of the system.
What to look for 10: Good Things?
If you see something nice in the PR, tell the developer
How to navigate a PR?
- General: Does the change make sense? Does it have a good description?
- Most important: Look at the most important part of the change first. Is it well-designed overall?
- Rest: Look at the rest of the PR in an appropriate sequence.
