"Code review" is one of those topics where it sometimes seems that everyone means something slightly different by the phrase, yet everyone assumes that others understand exactly the same thing as themselves.
Code review in the broadest sense just means reading code; inspecting code critical eye; usually code written by someone else at an earlier date.
The greatest benefit is that bugs are found early. Another benefit is that coders can learn from other's examples. And that common standards and idioms are propagated.
Wikipedia defines code review as "Code review is systematic examination of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving overall quality of software and can also be used as a tool to better develop skills at the same time."
Some degree of code review is cost-effective, but there must be a limit to that somewhere. The Extreme stance is that continuous pairing is cost-effective; but nobody is suggesting that we'd get software quality cheaper and faster by having ten full-time reviewers for each person engaged in coding. So, there are natural limits, but some code review is good.
Code review deals with the basics of software. I seem to have an interest in getting the basics right - it's not SOA architecture design, but I think it's important to get the basics right even then.
Types of code review:
I'll try and list some types of code review that i can think of, to make discussion easier:
Self review:
Reading your own code prior to check in is in my opinion a good practice, especially if done with the source control system's diff tool in order to see what you have changed.
Informal peer review
e.g. "Hey Pete, can you have a look at this code before I check it in".
Pairing:
Reviewing the code as it is being written. This is slightly different to regular code review, since one of the points of "review" is to see if the finished product makes sense, without seeing how it got like that. Also note that wikipedia's definition of review includes "overlooked in the initial development phase" – if it's during the initial development phase, it's not review, it's just view. I'm not saying that pairing is bad, just that it does not quite fit the definition of code review.
Round-robin peer review:
Each person on the team reviews one other person's code.
Group peer review.
A piece of code is selected, and all members read it and comment.
Senior review:
A team lead looks at the code of all other people on the team.
Automated review:
Fxcop and the like can highlight lines of code worthy of more scrutiny.
Repository review:
Inspecting the source control repository to see what has been checked in. This is more common on open-source projects where many people can check code in.
Patch management:
A more extreme version of repository review where changes are submitted as a patch to someone who is willing to review it and who has permission to check into the repository. In extreme cases, e.g. on the Linux kernel, there are multiple layers of review. The person who first reviews and accept the patch does not have access to the core repository, but must then pass it up the chain after acting as a quality filter, either accepting, rejecting or reworking submitted code.
Other parameters
Feedback can be: verbally in a meeting, via email, wiki or comments in the code, via the source control system (e.g. TFS policy).
Formality can be low, with an informal eye cast on the code, or formal with a meeting scheduled and minutes taken.
Mandatory review: TFS can be set up to force all changes to be reviewed by another party before being approved.
Issues document:
This is a wiki page or the like where issues are recorded for later refactoring. I have seen it called a "code review document" but I don't see it as such.
Problems: Writing down an issue gives a feeling that "something is being done" but serves to delay actual action.
It is harder to separate bugs from todos, which are not within the scope of code reviews.
Keeping a list of todos and issues to large to tackle immediately may well be a good idea, but I don't think it fits the definition of code review.
Code review checklist:
A list of items, some possibly found in previous code reviews, to look out for in further code reviews. Items can be anything from general standard and policy down to specific errors to avoid.
My experience
Group peer review has worked well for me on a past project. The procedure was this: early in the week a file of code would be selected – selected because it was important to review – e.g. critical infrastructure, or it was the most complex example of it's kind; or a typical example of it's kind, or the first example in a new area of code.
We would print out the code with line numbers, and scribble on it with marker pens and pencils.
On Friday afternoon we went into a meeting room and worked from the top, commenting on everything. Any issues that repeatedly came up were fed into the code review checklist.
It's as much about learning from the code as it is about spotting errors in the code. Though a surprisingly large number of potential sources of error were spotted.
Also a common coding style rapidly came out, which aided readability. After a while we were rapidly moving past things that were old hat – either recorded in the review document or that we agreed to disagree on.
I suppose that this could be done at less cost by not being so formal – not booking a meeting room and all that, but the very formality and rhythm of it helped ensure that it kept going for quite a while.
Like many agile practices, it both supports and depends on the people - if your team is fractured, this won't fix it, but make it more visible.
So, what is your experience of code review?