As we review more code, it is worth spelling out the expectations for the code review.
- Review the codes ASAP — this unblocks a lot of people. In fact I believe that code review is number one priority, as there are potentially a lot of people are immediately affected by it.
- Assume good intentions!!!
- Offer alternative implementations, but assume that the author considered them already.
- Most comments in the review are opinions. However, you cannot just brush them off — the resolution is reached through discussion.
- Ask questions, and don't make demands. You cannot just say “Call this variable foo". Ask why the variable was named that, and explain why foo is a better name.
- Ask for clarification.
- Avoid using “my code”, “my old diff”, “yours”, “not mine”, etc.
- DO NOT use words that could be referred as personal. It's a big NO-NO to say “This piece of code is stupid”.
- Be explicit — people don't often understand intentions online.
- Respond to every comment
- Don't use words like “always”, “never”, “it's easy” — this is hyperbole, and it doesn't really help.
- Be humble, and just say “I don't understand — please, explain”.
- If there are too many “I don't understand”'s — chat in person.
- If discussion becomes more academical/philosophical — move it offline.
- If you found something that is not-necessarily wrong, but it is your “pet peeve” — mark your comment as nit.
- Don't get offended by nit's — often times they are there to make the code more readable.
- Don't take it personally — the review is for the code, not you! :)
- Once you go through the code — mark it as either “Needs changes” or “Accepted”. There is NO third option, unless you are just asking a question.
- Clean, readable code is the key — if your code is not readable, noone will want to read it!
- Try refactoring the code while solving problems in the code.
- Write unittests — this probably should be most of your time :)
- Put the unittest invocation (how to call the test) + URL for the results that you already ran.
- Explain in the diff summary the details of the code — this will help the reviewer a lot!
- Put comments in the code in the parts where reviewers asked a lot of questions — if the reviewer didn't understand it, the user will definitely get lost.
- If you need to make changes to an “accepted code”, mark it as “needs review”, don't just change the code and land it.
- If the code is accepted, but there are some nit comments, feel free to change the code without “re-review” — that's why there are nits :)
- Sign off the diff/pull request with a thumbs up / “ready to merge”
- Announce breaking changes to all involved parties BEFORE landing!
- Put 1-2 reviewers for your diff. If you want to add more, put them as “subscribers”.
- Do not play a “gatekeeper” — you are here to give feedback :)
- Don't enforce your opinion on PR author/reviewer without giving explanations.
- Don't piggy-back big and/or breaking changes on an accepted diff — Either create another diff, or request a “re-review”
- Don't put the whole team in the “reviewers” list — it is usually detrimental
- Don't put your manager as a reviewer, unless you really want her/him to review it — they usually don't have time to review it, and it just creates noise. Put them as subscribers if you really want to.