Fog Creek Software
Discussion Board




Opinions On Code Reviews

Given the emphasis on code quality here, I
was wondering what the general opinion was
about code reviews. Who does them? Are
they cost effective (i.e., does the time cost
offset the time saved from caught bugs)? What
should be reviewed, and how should the review
be best conducted?

Thanks for your opinions.

Jessicab
Friday, April 18, 2003

We have code reviews, both formal and informal.  Both are worthwhile.

Before any code is checked into the source code control (SCC)  system, we must have a peer review the code.  This is our informal review.  We use the SCC's difference program that highlights all differences. The programmer that made the code changes explains why the code was changed.  More often than we expect, the programmer who has made the changes will find flaws, and can then go fix them.  Occasionally the person doing the review will find a few other problems, or can suggest a better way.

The formal reviews are not as valuable.  After a significant enhancement to the software, several people are pulled into a code review meeting.  Two members of management are included. The style and function of the code is examined.  Some bugs are found, and this makes the meeting worthwile.  One of the management reviewers always only seems to check for constants that are hard-codded in-line, and insists that we decalre them in an external header file.  Things like if(a[0] == 'q') exit(); -- He will insist that the zero in the subscript be declared externally in a constant, and the 'q' too. Usually his comments are misguided.  We nod, write something down on our notepad, then never adjust the code the way he suggests.  There is never any follow up by him that insures he got what he insisted on, and we are all happy.

In preparing for the review, the programmer with the new code finds bugs.  Additional bugs are found by the same programmer during the review. This makes the reviews very valuable.

XYZZY
Friday, April 18, 2003

In a good team code reviews are invaluable. There's lots of good reasons to have them and some bad too. Steve McConnell in Code Complete covers the area fairly well. The main issue is in getting useful results from the review, if all you are getting from your reviews is spelling errors and typos then that's bad, if you're getting better design and efficient variable scope and cleaner code then that's good.

There's definately a effort/return ratio that you need to find if reviews are going to occur frequently, I mean, you don't want to allocate 3 hours to a developer to review code and get a spelling mistake out of it, it's like everything, some people are really good at it and some people are hopeless.

Realist
Saturday, April 19, 2003

Things like if(a[0] == 'q') exit(); ...

The problem is that this could be an inadvertant assignment to a[0] if you left out an equal sign.  I must insist you reverse all your tests like "if ('q' == a[0]) " before this is released as production code ...

-- Alyosha`

Ineffectual Reviewer
Saturday, April 19, 2003

Code reviews are very valuable, if they are actually done and if the focus is on finding bugs instead of the naming of variables and constants or indentation.

Reviews will often reveal bugs that have a low probability of being detected in a testing cycle because the conditions that would cause them to fail are rare. In particular, reviews are extremely important if the code has nondeterministic behavior such as multithreading or database locking issues.  Running a multithreaded program successfully is almost no indication of whether the threading issues are programmed correctly, because you typically don't see a problem until two threads or two database sessions happen to hit the same variable or data at or nearly the same time.

T. Norman
Saturday, April 19, 2003

In addition, code reviews help to catch bugs earlier in the development cycle ... and the earlier they are caught, the easier and cheaper they are to fix.

T. Norman
Saturday, April 19, 2003

I should add this -- code review meetings are usually not a good idea, unless there is some real disagreement that needs to be resolved.  Just log them with some kind of tool or database, or use email, and leave the meetings for situations where there is a real disagreement regarding the code.

T. Norman
Saturday, April 19, 2003

I have had some code I written reviewed in a code review.
Meetings like this can be real exhausting, sitting reading code for long periods of time, but its still worthwhile.

I got my different algorithms and designs questioned even if they worked. Like why did you do X and not Y, and also we found a few bugs.

At first I found this questioning of my code kindof like hard to take critisism, but some of this questioning led me to rewrite the code for the better. Most of these rewrites were minor and the first implementations suffered because of my limited business knowledge at the time the code was first written.

While you will not find all bugs it will help catch a bunch that otherwise would have made it into production systems.

Patrik
Saturday, April 19, 2003

Think about it this way:

When there is a bug in production, what's going to happen? People will review the code to find the source of the problem!  Those post-production reviews can be very labor-intensive, often requiring looking at 10,000 lines of code to eventually identify a one-line change, plus regression testing after the fix is in place.

So you're going to end up spending time to review code anyway.  Do those reviews before production, and you'll save the effort involved in identifying, fixing, and re-testing. You also get the intangible benefit that results from programmers writing better code to begin with because they know it will be reviewed when they submit it.

NoName
Saturday, April 19, 2003

I find code reviews very helpful.

1) They definitely find bugs.

2) They are a useful way to assess the quality of code being produced by people working for the company. (Much better than finding out years after the fact.) It provides an opportunity for correcting poor practices early in the process (helping to make good practices more of a habit.

3) They reduce the sense of "exclusive ownership" that programmers (typically new ones) have with code they produce. It encourages the notion that the code is owned by a group and enhances consistency within the group.

4) They provide an opportunity for transferring experience, etc, to other people.

Basic formatting issues should be dealt with by running the code through a "pretty printer" program (like gnu's "indent").

Things like variable names and misspellings might seem trivial but carelessness here contributes to the degrading of quality of code over time. Spelling errors can also reduce the usefulness of text searching.

Ideally, the reviews of a particular person's code should get easier and less frequent (or detailed), over time

njkayaker
Saturday, April 19, 2003

I'm surprised nobody has yet mentioned the ultimate form of code review: pair programming. Anybody doing this? I find it extremely beneficial.

Brad Wilson (dotnetguy.techieswithcats.com)
Saturday, April 19, 2003

Does unit testing lower or remove the need for code reviews?

Thomas Eyde
Saturday, April 19, 2003

But code ownership is a good thing. You should encourage people to take ownership of part of the project - it promotes a feeling of professional pride in their work, which increases quality.

And the horse you rode in on
Saturday, April 19, 2003

Unit tests by no means eliminate the need for reviews. Testing can only cover a tiny subset of all the possible conditions and inputs.

If the unit tests are constructed by another person looking at the code to create test cases for full code coverage, reviews become less important. But that type of unit test construction inherently involves a level of reviewing, even if it's not called that.

NoName
Saturday, April 19, 2003

Shared ownership is good. However, selfish and defensive ownership is counterproductive.

T.Norman
Saturday, April 19, 2003

One reason I'd like to work in an environment where there are code reviews is to expose the poor quality of code produced by some developers.

If I'm working with difficult to understand, poorly written code it's going to take me longer than it should to complete some task that uses that code. If I'm assessed as being too slow because someone else did a poor job I'm not going to be very happy.

On the flipside, if I complain that the code I'm working with is hard to understand then a review by someone else (i.e. not me, not the person(s) who wrote the initial code) can verify if the problem is as I state or with my approach.

This goes back to addressing the mistaken assumption that all developers are "created equal"/are just code monkeys/etc.

Walter Rumsby
Sunday, April 20, 2003

"I'm surprised nobody has yet mentioned the ultimate form of code review: pair programming. Anybody doing this? I find it extremely beneficial. "

I'm with you on this.  I'm not of the opinion that 100% of the coding should be done this way, but for complicated or critical features, this is a great way to go. 

Norrick
Monday, April 21, 2003

Yeah, we don't do it 100% either. We do it for knowledge transfer and to get 2 sets of eyes on key code.

Brad Wilson (dotnetguy.techieswithcats.com)
Tuesday, April 22, 2003

There are a few rules that make code reviews effective:
- do not review too much code at a time.
- involve only 3-4 reviewers (one senior) in the process.
- keep meetings on topic, short and sharp.
- review only what is worth reviewing. Use tools.
- track down results
- learn and improve
- make it a habbit

CRs are most effective when you are used to "code a bit, test a bit, ...".

Finally pair programming IMO is approaching the right problem the wrong way: more eyes better quality! Instead better people, better quality!

Cheers
Dino

Dino
Tuesday, April 22, 2003

As a programmer, I believe code reviews are the time to hold programmers accountable for the work they are producing.

Juniors take note, there is a difference between style and structure.  No, global variables are not a matter of style people - they are simply inexcusable in most all cases.

Unfortunately, there are too many managers who have fallen behind in the times and have little understanding of complex, abstract concepts and best practices.  They should be building relationships with senior programmers who know this stuff and then using them in confidence to get value out code reviews.

If code reviews are for juniors, self-studies alike and dusty seniors, then they are useful because this covers about 80% of the working programmers.  Hence the growing amount of crap that ceases to perform as specified by the our employer - the user.

Thanks for reading.

AC
Tuesday, July 20, 2004

*  Recent Topics

*  Fog Creek Home