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

Yes they do.

[bluGill]

They need to be enforced from a high level, and a committment from management to take the time to do them. We have a mandatory checkless, no new feature can get into system test unless it is complete. On item is a code review. (Documentation is anouther) Test gets to decide what they will allow in test and if code reviews weren't done it isn't allowed.

Our cheif archatec insists that he review a lot of code to, and he is serious about it.

Code reviews are not allowed to last longer then 2 hours. If you need more time (and you almost always do!) then do it anouther day.

Two key problems that can come up

[KyleCordes]

Code reviews are of course a good idea, a "best practice", etc. They can be hard to actually do for a variety of reasons... here are a couple of key ones:

1) A code review is essentially judgemental. Even when the reviewer(s) are careful to "review the code, not the coder", the nature of the interaction is that person A looks at person B's work product and perhaps finds it less than good. This can be uncomfortable for the reviewer (who doesn't want to say bad things about a coworker's code) and the reviewee (who might be hesitant to see or admit weakenesses in their work product). Reviewers often hold back valid issues for this reason.

2) Reasonable people can disagree. Sometimes it's the difference between a weakness and a design decision is unclear.

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

Re:They do exist - and yes they are very useful.

[dead_penguin]

...In my current job, I submit code to review all the time, but none of the reviewers actually understand it...

In that case, I'd say there are a few serious problems with your working environment. I'll bet that at least one of the following apply:

1. Your code is being reviewed by people that are not programmers. I can't really see anything too productive coming out of this. If you're working at a place where unqualified people are conducting code reviews, maybe you should start looking around a bit for other opportunities

2. You're obfuscating your code (either on purpose, or just out of habit). Anyone who is not commenting their code decently (excessive verbosity is almost as bad as not commenting at all!), or who is writing muddled, unclear code, is hurting the team and company more than helping it. There is a reason code standards exist, and while nobody likes being told how to style their code, they work.

3. You're using Perl. ;)

Ok, I just don't like Perl. I know it's got lots going for it, readability and consistency aren't some of them. (I'd better stop now before some militant Perl fanatics show up at my doorstep...)

"Intelligence is the ability to avoid doing work, yet getting the work done".

yes, but rare

[Mr. Slippery]

I've worked in two environments where code reviews took place.

When I worked at TIS on the Trusted Mach project, code had to be reviewed by the "trust engineers" for compliance with the security model. Problem was that some of these folks, while very knowledgable about the theory of building a trusted operating system, were not all that knowledgable about C++, and a lot of their problems - in some cases, the vast majority - were cleared by developers explaining basic programming issues.

A few years later I worked at TRW on EDOS. Code reviews were a mandatory part of the process. Unfortunately, in my experience they ended up more focused on ensuring that the code met the style standards (indentation, brace placement, format of function headers, and the like) than on the proper functioning of the code. (Of course, that may be because my code was just so good that no one had any suggestions, questions, or issues other than where I put the curly braces. B-) )

Tom Swiss | the infamous tms | http://www.infamous.net/

My experience

[dubl-u]

My experience with code reviews is that they are helpful when people actually do them. But they've always felt very artificial to me; even though I know intellectually they pay off, they feel like they take me away from "real" work. And I've never done it in a context where all code is reviewed; I've only seen it done where a sample is taken and examined.

When I've done pair programming (in an Extreme Programming context) I feel like I get most of the benefits of a code review, but over 100% of my code. And since the reviewing is integrated into the coding process, I always feel like progress is being made. Indeed, now solo programming feels a little unsafe to me, like swimming in the ocean with nobody around to notice if I get sucked out to sea.

passive code reviews

[Jim Winstead]

i've always been a fan of what i'd call passive code reviews, and its something you see all over the place in open source projects -- diffs of cvs checkins mailed to all developers. i used this when managing/working with small teams of programmers on a few projects now (one a consumer software product, the others web-related), and i found it to be tremendously beneficial and unobtrusive. with a little practice, reading a diff is a very quick process, and the number of stupid little bugs you can pick up is just gravy on the top of getting a great feel for what other people on the project are doing.

that said, i'm sure the formal code review has its place. but who really wants to work that way?

Yes and No, Good and Bad

[staplin]

Code reviews exist and don't exist primarily as a function of the company you're at or the group you're in. And if they do exist, there's a whole range of quality to them.

I've worked at a large government contracting comapny, and they had code reviews as part of the ISO process. Which meant that they occured regularly. And I've also worked at a small company, where everyone said "We should do reviews" but then never did. I take that back... they tried one, no one prepared for it, so it flopped, and a follow up was never scheduled.

Now I'm not saying that preparation is absolutely necessary. If everyone is familiar with the code, they can do a good impromptu review. But if you have no clue what the code does, you probably won't be able to comment on anything but the style , unless the code is pretty rudimentary. Also, giving people a chance to read through it at leisure lets you have a relatively quick meeting as people run down their red-lines rather than sitting and reading code in a conference room.

And when you actually review, review the code, not the coder! Some people tend to turn code reviews into an ego driven contest. They may not release any code until no one will be able to find a single bug when it comes up for review. Others may pore over code, desperate to find any bugs they can, in order to bash the reviewee. Once you get into this kind of situation, no one is productive since they're way to concerned with the reviews to actually do anything else.

But I've also seen very good reviews where constructive criticism came up, suggestions were given to improve architecture or code flow, and people on both sides learned something. And that's what you have to keep striving for, because it may not come very easily.

Design review at my last company

[MrBlack]

I was hired by my last company to design and implement a new module for their 5-tier enterprise ready product. After I'd finished the design the development team did a design review. It was a bit daunting, since I'd only been there for a month or so, to stand up there and explain my design, why I'd made the decisions I had etc. I found it quite helpful. The other developers pointed out a few areas where things needed to be changed to interface well with the current system, but I got a few funny looks for some of the things I said. It was only after I started to integrate my stuff that I realised why. Although the subscribed to all the right software development practices in theory, the paid no heed to them in practice. GUI code passed raw SQL queries right up to the database layer to be executed (so much for the 5 tiers). Encapsulation was broken all over the place. I'd call it cargo-cult object oriented programming. I was retrenched 90% of the way through implementing the project as the company started to run out of money (first in, first out, you know). During the handover I explained how the implementation worked to the person who was going to be taking over from me (who was quite good - he was from a smalltalk background so he was painfully aware of all the things we were doing wrong). After showing him how it worked he said something like "yeah, that's how I thought all of the system should have been done." Anyway, enough of my war stories. In short - I found design reviews a good thing - especially when you're working with a system you don't understand well. They can catch problems with your design early on.

They do exist - and yes they are very useful.

[tjwhaynes]

Pretty much since I started work at some company you might be able to guess from my signature, almost every major piece of work added into the source tree gets code reviewed in some form or other, from simple process flows through to line by line analysis depending on the likely critical impact if there is a failure.

Does this procedure help? I'd say it does - especially with a product as large and complicated as DB2 UDB, there may be side effects that are not obvious to just one programmer. There may be logic problems that you have missed which are clear to another person. Bad coding style leading to problematic maintenance problems in the future can be caught.

Just as nobody would dream of submitting a major article for publication in academia without having it proof read by at least one peer, it doesn't help a large product to have large volumes of code stuffed into it without somebody other than the original author looking over it. So while I used to find the process of code reviews cumbersome and time consuming, I now feel that code reviews save more time and energy in the long run.

Cheers,

Toby Haynes

Y2K

[Prior Restraint]

Late 1998, I did contract work at a long-distance company. I worked on a Y2K-compliance team. We did weekly code reviews, just to make sure nobody missed anything, or broke more than they fixed. That was the one and only time I did a code review outside academia.

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. :)

Seldom, but they're worth the time!

[MadCow42]

I know that it's tough to schedule the time to do code reviews in a hectic development schedule, but my company has definately seen the benefits.

We've started the (sometimes painful) habit of doing a "post-mortem" of each release, and start the next round of coding with a code review. Although it would be better to do it 3/4 the way through the development cycle, we've found that this is the only place we can "squeeze it in" and still be able to spend sufficient time on it to be beneficial.

There's nothing better for catching potential problems, learning other areas of the system better, and forcing people to justify their coding practices (or change them for the next round) than a code review.

We've also found that it helps the junior programmers by having a more experienced person evaluate their code, and make suggestions.

So, it's more of a post-mortem analysis, sometimes too late to actually end up with improvements to the current code, but it's helped our team prosper, and sets the pace for the next iteration.

MadCow.

Yes, and they can be useful

[NaturePhotog]

When I worked at Geoworks, we did code reviews. Not all the time or for every project, but they were done regularly for:

• engineering co-ops
• newer programmers
• changes to core code
• bug fixes done near deadlines

This last one kept more than one serious bug from going out the door. When the pressure is on, there's a temptation to make "just a little fix" that frequently would introduce a new problem. When a freeze date was close, the build manager for a project would review any changes before allowing them to be installed. Code reviews were also very helpful for engineering co-ops and other newer programmers. As much coding as you've done in school or on your own, you were probably the only one besides the grader that's ever seen it, and graders usually weren't looking for good code, but for code that worked the way the professor wanted it to (I had one exception to that; the prof for my OS course required following a coding convention.) A code review for newer programmers helps more experienced programmers impart their wisdom about coding conventions (every large project should have one), choice of data structures and algorithms, and documentation & comments (every large project should have some :-) In part, this is how open source works. One person writes some code, and other people look at it and find bugs in it, better ways to do what it does, ways to change it to make future expansion easier, etc. For a larger open source project like, say, Linux, many more people look at the code and it gets better and better as a result.