Code Reviews- Do They Really Exist, In Practice?

dante101vr2 asks: "While working in the programming field I have heard a lot of mention to doing code reviews, however I have yet to see one occur. Along with my job, I have also talked to a number of others throughout the IT community who laugh when they hear mention of a code review, claiming that there is not enough time in the day. My question to you is, do code reviews exist, and are they a good tool or are they just time consuming?" For those of you who do have normal code reviews, what small things do you do to make the whole process go along as painlessly as possible?

Comments

Rare, but they can work very well.

[Lumpish Scholar]

Code reviews are the practice most univerally praised and most rarely carried out.

My first exposure to something like code reviews was writers' workshops; everyone submits a poem, story, whatever, and then everyone critiques everyone's works. From this, I learned the first rule of both writers' workshops and of code reviews:

REVIEW THE WRITING, NOT THE WRITER

Being reviewed is tough on the author's ego; spare him or her as much as you can. If everyone gets a turn in the hot seat, they'll learn the difference between flamage and constructive criticism.

Here's the second rule: People hate performing less than they hate preparing for code reviews. It's heresy, but I've had a lot of luck with zero-prep code reviews: Everyone shows up, gets a printout, and away we all go. It can take a lot of time in the review, but the total time spent is smaller (and, IMHO, more effective). In reality, people skip the preparation; why not plan for it?

Third rule: Reviews identify problems, not solutions. You don't have to know a better way to know something's wrong.

Fourth rule: The author is there to learn, and to answer questions, not to be defensive.

I brought code reviews into the small startup I joined a few years ago. Final rule: everything deployed to production gets reviewed. It worked; not only did my co-workers accept it, but at least one of them, when he left for another job, got people there to do code reviews the same way.

Code reviews aren't just for finding bugs; they're for improving the quality of the code, for whatever your local definition of "quality" is. I've always considered maintainability to be fair game.

My current project has a fairly rigid process, put into place long before I arrived. Here, they do desk checks and call them "reviews." It's not as good, but it's better than nothing.

Well, we tried.

[dmorin]

First off, don't let non-coders run code reviews. I have a guy on our team who claims to run code reviews where he packs the whole team into a room for 8 hours and they go over what basically amounts to business cases, checking the code to make sure they are covered. That's not quite what I call code review, I dunno about you. Especially since the project managers and everybody are in there.

I've done "mini reviews" where I randomly grab code from the repository, and when I find something that looks interesting, I start a message thread to discuss the pros and cons of it with the team.

Code review for us meant inviting 5 people : the original code author, a moderator, and 3 reviewers (although the moderator can also be a reviewer). Everybody was told what code would be reviewed, and given printouts so they could read up. Then we took 2 hours where the moderator runs through the code and the reviewers offer comments to the author. I think we did somewhere between 1000-2000 lines of code that day. The end of the code review is a list of items that can be changed in the code, broken down by priority -- should be, might be, etc.. Don't feel like you have to put the whole team in a room! It will be impossible to schedule, and you won't get through any code at all because people will spend too much time arguing about the details.

Don't invite the boss until you've got a good process in place. Otherwise people will wait for the boss (who is probably not a fulltime coder) to lead the meeting, when in reality he probably just wants to listen. The idea is that it is a peer review, not a criticism by your superiors. Likewise, have a good mix of junior/senior programmers -- everybody's opinion counts. When I ran the above example I put names in a hat and drew them randomly.

Making it mandatory is important, in my book. You need enough reviewers to make the discussion interesting.

Also, decide in advance what the scope of your code review is! Some people will want to talk about indenting styles that they don't like, or schemes for naming variables. Is that what you want to talk about? Do you want to focus on things like uncaught exceptions and null pointers? Or something broader, like whether a particular path through the state machine has been handled properly? You have to tell them up front, otherwise you'll get a wide range of opinions on the level of detail that you should be discussing.

If people aren't used to code reviews, it's important to explain to them that the benefit comes in solving the problem before it happens! When you see a method that doesn't have a scope declaration on it, it looks completely trivial to point that out. But it's saving you time later (I really did have a case where we'd created a new package and were trying to extend some old code from a different one. The junior programmers on the case spent quite awhile (days?) figuring out why it didn't work, and finally decided to just not use a new package. Turns out that a good part of the code in the old package was using the default package-private scope and thus not extendable into a new package).

Unit test! If you can't get people to review the code, get them to review the unit tests.

Wherever possible, review new code. The biggest problem I have here is that we've got a 3 year headstart on writing code without reviews, and now I can't rally anybody into meetings to go back and review existing code. When I do, any changes that are needed don't become a high priority. Make it a point that when new code is being created, just make the code review a part of that process.

Duane

30 years, regular reviews at all levels

[satch89450]

Portions of my comments will seem redundant, but I think that there is enough new material for the entire essay to be useful. I'll see how the moderators feel about that, of course. :)

My experience extends over three decades, and I'm here to say that product implementation reviews are indeed real, in academic and industrial enviornments.

First off, the review cycle is more than just reviews of the code -- the best projects had reviews at every major milestone. This was especially true of those milestones that required the ball be handed off to the next group to work on the project.

Some reviews are major deals: marketing definition review, product design review, and development assignment review. Some reviews are relatively minor, but just as important: detailed design reviews of hardware and software, post-implementation reviews ditto, and integration reviews. The difference is the number of people involved. Major reviews required a fairly large conference room, and in the instance of development assignment the companies I worked for would book a large ballroom to hold all the people involved in the project. (Smaller projects took over the lunchroom.) The other reviews were two- and three-people affairs: peer review and critique would have you handing over notes and source to a peer, and the two of you plus the supervisor (task lead, group lead, whatever) would go over the results. Integration reviews may involve as many as five to ten people, depending on the level and diversity of the elements being integrated.

Then there were the bug-track reviews...

By the way, in one small company the peer reviews were very informal -- sometimes the review would be done over a cup of coffee during a break, especially when the patch was small and localized...but the review was done because it saved so much grief down the road. Especially grief from The Boss.

There was another part to reviews in the small company that was lacking in the large companies I worked for: documentation reviews. Part of the job at the small company was that any change to the code required changes to the corresponding interface, control block, and other design documentation. No change was allowed to go through without the documentation changes being included in the change report. It was the only way for eight programmers to be able to provide maintenance support for 19 man-years of assembler code AND be able to also develop new products.

The best review system? The one that let people learn about and fix mistakes with the minimum of fuss. At ALL levels. One of the first things I learned was that promoting the people who made the fewest mistakes was a recipe for disaster, because it penalized people for being aggressive with implementation so that the company could satisfy customers. Granted, sloppy programming should be frowned on, but the whole point of the review system was to let the system catch any mistakes so each person didn't have to waste time going through code sixteen jillion times to try to find that "last bug". Better to let a new pair of eyes catch it quickly.

When you analyse all the "systems" of developing programs that have been promoted over the past five decades, it comes down to having multiple eyeballs catch the silly stuff and the tricky mistakes, and to not allow sloppy stuff to even enter the stream. For if you know that your code will be seen by others, you take pains to not only get it right but also to get it pretty.

This last, to me, is one of the benefits of OSS development -- you will be judged by a LOT of people. :)