I recently wrote some thoughts for improving code reviews within my team. It started like a 5 minutes brain-dump at 6:30 in the morning on my way to work and I ended up iterating and refining the list several times over a few days. This gave me the opportunity to reflect on the differences between the code review processes among the various places that I worked at. Also, I strongly believe the environment (academic, industry) matters a lot. I wanted to share these thoughts as code review can be the cause of many tensions within a development team.

There are three types of rules: general ones that apply to both the submitter/reviewer, submitter rules and reviewer rules.

These guidelines are not perfect, take them as they are: an attempt to formalize what I believe we should shoot for.

General Guidelines

  1. Always assume good intentions. People are not commenting on your code to harm you. They are trying to help you.
  2. Be thankful for having reviews, people are dedicating time to look at your work. No matter the experience of the submitter and reviewer, there is always a chance it can make you a better programmer or co-worker.
  3. Always try to understand first, and then, be understood. This comes from the 7 habits and is especially helpful in that context.
  4. During a conversation, get the perspective of the other person, try to put yourself in his/her shoes. This will often help you understand why a comment is made and help you discuss and resolve it.

Reviewer Guidelines

  1. Understand there is more than one way to do it - (c) Perl -. The goal of code is to solve problems. A problem might have several solutions that are equally effective. Acknowledge and accept that somebody might choose to solve a problem differently than you would do.
  2. Specify explicitly if a comment/problem is blocking the code to be shipped: some code review systems allow you to distinguish blocking/non-blocking issues. Try to make use of it so that the submitter can prioritize changes.
  3. Keep discussion/comments in the review system: it also serves other members to learn about potential issues and not replicate them. Ultimately, if you need to talk in person (sometimes, there is nothing like a face to face discussion), do it but write the outcome in the review for traceability purposes.
  4. When you point out a flaw in the code/approach, try to propose an alternative: telling somebody that his/her code is bad is not very useful if there is no indication for fixing the issue. Try to explain why this is bad and give some ideas how to address the issue. If this is a simple issue (syntax error or coding style violation), just mention this without spending much time to give details.
  5. If the review matters to you, do it soon, probably within 24 hours and the exact delay is something a team should discuss. It is better for everybody to get it done quickly: all ideas are still fresh in the head of the submitter, it does not delay progress on new features, etc. Do not blame the submitter if some code has been shipped without you reviewing it if it was for review for several days: the blame is probably to put on you for not reviewing it in a timely manner.
  6. When discussing or arguing a change try to relate to coding guidelines, best practices or business goals. There is no point requesting a change if you “just prefer this style”: you should have a valid, objective reason to request that. For example, I personally almost always consider readability and maintainability as one of the most important aspects of code (software maintenance and developer time is more expensive than computing resources). But sometimes, good readability must be impacted to favor performance: in some businesses I have been, winning few nanoseconds on a code path matters more than having clean and readable code.

Submitter Guidelines

  1. Link your change your issue tracker (Jira, github, etc.) and follow the rule that 1 code change belongs to one ticket (one ticket can have multiple code changes). It improves traceability with user stories and facilitates progress tracking on a particular task. Design decisions, issues, etc. should be discussed in the ticket, not in the review itself.
  2. Make short reviews and try to keep one code change per problem and per component. First, it avoids long code review that will take days to complete and will ultimately cause merge problems (need to re-base on master, etc.). Second, reviewers are less likely to complete a long code review in a short period of time, which ultimately, will slow you down. Prefer several, incremental reviews to a larger reviews on dozens of files.
  3. If there are some temporary code that will be changed later: put a comment in the code with your login that explains why this is delayed and when this should be addressed. Having a random FIXME without reason is not acceptable, having temporary incomplete code is OK as long as we have a reason for it and a plan to address it in the future.
  4. Address comments in the review: people invest time doing review, respects the work they put in your review and answer their comments.
  5. If you are doing pair programming, the ship from your pair does not count: code review is there to expose the code to other opinions and people in your team. Code developed in pair lacks this external point of view. Having the pair shipping the code is like if the code was being shipped by its author. It defeats the purpose of code review.
  6. Specify how you tested the components in your review: some review system allow you to specify how you tested the software. Copy/paste the command you used to run the test. If you deploy to a testing environment, put the commend you used. It has two purposes: show the level of testing you did and, ensure that the software still deploys and run (at least in the testing environment).
  7. If one comment upsets you, come back to it later: one particular comment might irritate you and after reading it, you may want to reply in a sarcastic way or to just ignore it. Do not do any of these things. Take some time, try to have the perspective of the reviewer, and then, reply. Stick to the point. Try to understand why the reviewer raised that issue and show your good faith about solving this issue.