11

We are trying out mandatory code review on each commit -- nothing gets into master that hasn't been validated by at least 1 person not the author -- for a couple of sprints. We have buy in from both developers and management (which is an amazing situation to be in) and we want to get some of the benefits that it is known for:

  • obvious bug reduction
  • more awareness of changes happening around the project
  • the "I know someone is going to look at this so I won't be lazy"/anti-cowboy effect
  • increased consistency within/across projects

But we're introducing something that is known to reduce velocity, and if done wrong could create a stupid bureaucratic step in the commit pipeline that doesn't do anything but take up time. Things that I'm concerned about:

  • reviews devolving into just nit picking
  • (hyperbolically) people opening up huge architectural issues as part of a two line commit review.
  • I don't want to bias answers with other things.

While we're all reasonable people and we'll be doing a lot of self analysis, we could definitely use some battle-won insight into what kinds of things we should be trying to accomplish in a review session to really make reviews work for us. What are some guidelines and policies that you have found to work?

closed as too broad by gnat, Bart van Ingen Schenau, GlenH7, user40980, ozz Dec 16 '13 at 16:10

Please edit the question to limit it to a specific problem with enough detail to identify an adequate answer. Avoid asking multiple distinct questions at once. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

13
  1. Make reviews short.

    It's hard to stay concentrated, especially during code review, for a long time. Moreover, long code reviews may indicate either that there is too much to say on the code (see the next two points) or that the review becomes a discussion over larger issues, such as the architecture.

    Also, a review should remain a review, not a discussion. It doesn't mean that the author of the code can't reply, but it shouldn't turn into a long exchange of opinions.

  2. Avoid reviewing code which is too bad.

    Reviewing low quality code is depressing both for the reviewer and the author of the code. If a piece of code is terrible, a code review is not useful. Instead, the author should be asked to rewrite the code correctly.

  3. Use automated checkers prior to the review.

    Automated checkers avoid wasting precious time pointing mistakes which can be found automatically. For example, for C# code, running StyleCop, Code metrics and especially Code analysis is a good opportunity to find some of the errors prior to the review. Then, code review may be spent on points which are extremely hard to do for a machine.

  4. Choose carefully the persons doing reviews.

    Two persons who can't bear each other won't make a good review one of another's code. The same problem arises when one person doesn't respect the other one (no matter whether it's the reviewer or the author, by the way).

    Also, some persons are just unable to see their code reviewed, so they require specific training and preparation to understand that they are not criticized and they shouldn't see it as something negative. Doing a review, unprepared, won't help, since they'll be always on a defensive and won't listen to any critics of their code (taking every suggestion as a criticism).

  5. Do both informal and formal reviews.

    Having a checklist helps in being concentrated on a precise set of flaws, avoiding to devolve into nit picking. This checklist can contain points such as:

    • SQL Injection,
    • Wrong assumptions about a language which can lead to errors,
    • Specific situations which can lead to errors, such as operator precedence. For example, in C#, var a = b ?? 0 + c ?? 0; might look fine for someone who wants to add two nullable numbers with coalesce at zero, but it isn't.
    • Deallocation of memory,
    • Lazy loading (with its two risks: loading the same thing more than one time, and not loading it at all),
    • Overflows,
    • Data structures (with errors such as a simple list instead of a hash set, for example),
    • Input validation and defensive programming in general,
    • Thread safety,
    • etc.

    I stop the list here, but there are hundreds of points which may figure in a checklist, depending on the weak points of a precise author.

  6. Progressively adjust the checklist.

    In order to stay constructive and useful over time, checklists used in formal reviews should be adjusted over time depending on the mistakes being found. For example, first informal reviews may reveal a certain amount of risks of SQL Injection. SQL Injection checking will be included in the checklist. When, a few months later, it appears that the author is now very careful about dynamic vs. parametrized queries, SQL Injection may be removed from the checklist.

  • -Any examples of what should go on a code review checklist?- Let me google that for myself. – quodlibetor Dec 9 '13 at 19:21
  • @quodlibetor: I edited my answer to include a few examples. – Arseni Mourzenko Dec 9 '13 at 19:43
2

We have almost like a checklist:

  • Show me the task description.
  • Walk me through the result, and show it working. Run different scenarios (invalid input, etc).
  • Show me the passing tests. What's the test coverage like?
  • Show me the code - that's where we're looking for obvious inefficiencies.

Works fairly well.

0

I think a person who has power over the others would be enough, administrator or moderator to cut irrelevant comments, speed up reviewing things that need quick review. Single decision maker.

Minus of this would be that this person has to do it as main task, while he could be doing something else, and probably you would like to have most experienced person on this position.

Second thing is automating as much as you can!

  • controling white spaces
  • style controling software
  • automated builds before code review
  • automated testing before code review

Those things will remove at least some things that people might comment without real need. If it is not building or have trailing whitespaces it is not good enough for review, fix it and apply for review again. If it is not building or some test fail then it is obvious that it is not good enough.

Lot of it depends on your technologies, but find what you can check automatically the more the better.

We have not won this battle yet, but that is what we found useful.

  • We're doing this peer-style, nobody has absolute power to commit/block a change. If there is disagreement we'll appeal to group consensus. That will cause slowdown but also hopefully increase cohesiveness of everyone's coding. – quodlibetor Dec 9 '13 at 20:58

Not the answer you're looking for? Browse other questions tagged or ask your own question.