This is a living document, liberally plagiarized from Thoughtbot, Google, and others.
Effective and quick code review is a vital part of software development lifecycle.
You are free to copy, modify and reuse as you like. I often commit a code review doc in git repos.
- Speed is the most important. As soon as you have a mental break, review any outstanding PRs (pull requests aka merge requests) assigned to you.
- Keep your PRs small!
- Use auto-formatters, linters, and static analysis tools. CI should fail if it doesn’t pass these checks. This reduces “bikeshedding” and limits style comments which have little business value.
- If you have an urgent PR, reach out to the reviewer directly.
- Ask questions. Don’t make demands. Be kind. Assume good intent.
- Tune your notifications so you’re reliably notified of new PRs. Don’t make the author reach out to you directly for every PR.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
- Ask good questions; don’t make demands. (“What do you think about naming this
- Good questions avoid judgment and avoid assumptions about the author’s perspective.
- Ask for clarification. (“I didn’t understand. Can you clarify?”)
- Avoid selective ownership of code. (“mine”, “not mine”, “yours”)
- Avoid using terms that could be seen as referring to personal traits. (“dumb”, “stupid”). Assume everyone is intelligent and well-meaning.
- Be humble. (“I’m not sure - let’s look it up.”)
- Don’t use hyperbole. (“always”, “never”, “endlessly”, “nothing”)
- Don’t use sarcasm. It’s often lost in the written word.
- Summarize any outside discussions in the PR.
- Waiting 24 hours to have your code reviewed should be rare. If no one has reviewed your code within 24 business hours, you may merge the code without approval.
Having Your Code Reviewed⌗
- Keep your PR’s small whenever possible (< 500 LOC). Small is digestible.
- You (not the reviewer) are responsible for merging the code after code review approval if you have merge permissions.
- Be grateful for the reviewer’s suggestions. (“Good call. I’ll make that change.”)
- Keeping the previous point in mind: assume the best intention from the reviewer’s comments.
- Explain why hacks/shorcuts/abnormal/weird code exists. (“This code is obtuse because performance is critical here.”, “I have to get this out quickly to fix a critical bug.”)
- Try to respond to every comment.
- Merge once you feel confident in the code and its impact on the project.
- Be aware of how hard it is to convey emotion online and how easy it is to misinterpret feedback. If a review seems aggressive or angry or otherwise personal, consider if it is intended to be read that way and ask the person for clarification of intent, in person if possible.
- Talk synchronously (e.g. chat, screensharing, in person) if you have lots of questions about someone’s PR. Don’t litter the PR with lots of comments. Later, summarize your discussion on the PR.
- If the author does not have merge permissions, you are responsible for merging their code.
- Focus on what’s important - architecture, data modeling, error handling, etc.
- Don’t have too many nitpicks (misspellings, style issues, variable names, etc). Too many nitpicks can become frustrating and take the attention away from the important parts of the review.
- Point out some nitpicks but still approve the PR if there are no serious issues. Trust the author to fix the nitpicks before they merge.
- Communicate which ideas you feel strongly about and those you don’t.
- Compliment / reinforce good practices.
- Offer alternative implementations, but assume the author already considered them. (“What do you think about using a custom validator here?”)
- Pull down their branch and test out an alternative yourself. Fix it and push a commit for the original developer to review.
- Seek to understand the author’s perspective.
- Don’t ask the author to fix a problem they didn’t cause.