tags:

views:

182

answers:

4
  • Who should be reviewed?
  • Who should do the reviewing?
  • What code should be reviewed? (all code? Big changes? Etc)
  • Where should the review take place? (Does it have to take place in person?)
  • When should reviews take place? (Incrementally? Before check-ins?)
  • Why should code be reviewed?

I have some opinions about this but I will post an answer with those.

A: 

Everyone, everything... pair program! would be one answer.

However, I think it depends on the type of business you are in and how serious you are about the quality. I've worked in a medical equipment company and code review there was conducted periodically by going over pages and pages of diffs along with UML diagram by the chief designer, QA department etc.. See for example, US FDA/CDRH: General Principles of Software Validation.

There are several answers to why?

  1. To catch invalid requirements.
  2. To catch software design that does not fit the requirements.
  3. To catch implementation that does not fit the software design.
  4. To catch non-standard implementation.
  5. To catch mistake in implementation.
  6. To catch potential side-effect in implementation because the coder did not take in account of other modules.
eed3si9n
+1  A: 

Who should be reviewed?

Everyone who is developing code to be released into production

Who should do the reviewing?

The entire team

What code should be reviewed? (all code? Big changes? Etc)

This one depends. We review all code changes. Property and ini changes are left un-reviewed unless we deem it necessary after looking at the code.

Where should the review take place? (Does it have to take place in person?)

Yes, it must take place in person. Besides looking over code, you can pick up valuable information on general coding practices, IDE tricks, the list goes on. Reviewing in person also gives the coder a chance to explain why they did something that way, leaves more room for debate.

When should reviews take place? (Incrementally? Before check-ins?)

We do ours a week before the code is to be put into production. Everything is checked in by that point, and it leaves plenty of time for any issues to be corrected.

Why should code be reviewed?

The list for this question is ginormous. First and foremost is to catch code that can produce errors or bugs in production. Other things like catching non-standardized code, sloppy code, anti-patterns. Pretty much you name it.

Ethan Gunderson
A: 

The question has already been answered extremely thoroughly. But I'd just like to add that even if the code is fine, (seemingly) bug free, and follows standards, code reviewing has merits. Namely, it increases the truck number on the functionality that the dev implemented. (If only marginally.)

I am not, however, suggesting that code reviews should replace proper commenting.

Rob Rolnick
+5  A: 

Who should be reviewed?

Everyone who submits code into a shared repository.

Who should do the reviewing?

  1. A mentor or senior engineer who will look out for bad smells and errors in architectural correctness of the code.

  2. Peers on the team or pod working in the same area of code i.e. if you are working on 3d rendering in a computer game, all the other graphics programmers should review the code.

  3. Anyone else whose modules are interfaced by or dependent on the code being submitted.

What code should be reviewed? (all code? Big changes? Etc)

In my experience all code that is going to be worked on by more that one person (and that's almost all code).

Where should the review take place? (Does it have to take place in person?)

Reviews are most beneficial when they are interactive and real time. Having said that I think it's important to have a system like CodeReviewer to allow easy distribution of diffs so that reviewers can study the changes more efficiently and more comfortably within their own dev environment, before doing the actual face to face review.

I'm in no way associated with CodeBear, but I've always found my code reviews to be more effective when I've been able to view the changes on my own machine since I've got the IDE and tools and everything else configured just the way I need for me to effectively spelunk the code to see what impact the code changes will have and that is something I can't do if my review is all of 15 minutes at another person's PC watching them scroll through their changes.

When should reviews take place? (Incrementally? Before check-ins?)

Before check ins. I've been in situations where incremental code review amounted to me writing the person's code for them "incrementally" during each review. :(

Why should code be reviewed?

Two main reasons come to mind.

The first one is to preserve the quality and consistency of the code base. For any real world code base you will most likely have programmers of varying levels leaving and joining the team that's responsible for maintaining that code base and reviews try to minimize the code base becoming a reflection of the teams dynamics e.g. 2 crappy programmers joined the team last week and now 20 source files have crap in them.

The second reason is to remove tunnel vision assumptions in the code. At some point a code base will become too massive for one programmer to understand it all. And from that point forward everyone working on the code will have their own "slice of reality" about what the code does and how the system works and this will often lead to code being written that's based on naive or wrong assumptions. So having other people bring their view of reality to your code during the review helps reduce this problem of code base myopia.

grok