Code review guidelines: the "unwritten" rules
Code reviews are an invaluable tool for keeping high code quality within a project. Even though, at first, sound like a waste of time - why should I read somebody else's code when I can add a new feature? - they offer short and long-term benefits.
First of all, over the short term, they help with enforcing a common code style within a codebase. In addition, and maybe most importantly, they help a team keeping in sync with what is happening on different parts of the codebase (given a reasonable small team).
Over the long run, code reviews help with distributing knowledge among the team. An experienced engineer making remarks to a less experienced engineer will help build a stronger overall team experience. The higher quality code that you get from consistently doing code reviews will help a team to move faster when adding new features, in contrast with the initial perception that the code review process is hurting the velocity of development.
But having a healthy and useful code review process within a project requires some "unwritten" guidelines that both reviewers and authors need to follow.
I think that the most important bit to remember for both reviewers and authors is that code is not objective and black-and-white. No pattern or approach fits in all approaches. In addition, personal coding styles and preferences will always differ. As long as the project code style is respected, there's room for personal expression.
As a reviewer
- Never comment on the author or the author's skills. Comment and focus on the code itself.
- Assume the best intention from the author. If you see something that you strongly disagree with, politely ask for clarifications or context.
- Always remember that you are "suggesting" rather than "demanding" a change. So use appropriate language (e.g. starting with "Why not ...", "What about ..."). Of course, if something is plain wrong you should object but do so politely.
- When suggesting a change due to code style or best practice try to refer to the "official" doc. It's always better to back your suggestion with "evidence" rather than the "I said so".
- Comment with specific feedback rather than general advice. Be specific on what change you expect to see on the next revision. If a suggestion is optional or minor, be sure to mark it as one (e.g. prefix with "Optional").
- Don't forget that you are "allowed" to praise as well (you are actually encouraged to :). Look for good points and praise the author. Especially if the reviewer is more senior, a positive comment will help the more junior engineer to repeat the praised pattern in the future.
As an author
- Never comment on the reviewer or the reviewers' skills. Comment and focus on the code itself (the same applies to the reviewer - I repeat this because it's the root of most misunderstandings).
- Always take a second look at your code before sending it for review. Your reviewer will spend time reviewing your code so respect that time and tide up your code as much as you can.
- Reply to all your reviewer comments, even if it's just to say "done". If you disagree with the suggestion, explain why you think your approach is better.
Hopefully, this post might be used as a basis for your project's "unwritten" code review rules. Take care and happy coding!
P.S. Excellent discussion and additional points in this Reddit thread.