Slashdot Mirror
Are Code Reviews Worth It?
JamaicaBay writes "I'm a development manager, and the other day my boss and I got into an argument over whether it's worth doing code reviews. In my shop we've done both code reviews and design reviews. They are all programmer-led. What we've found is that code reviews take forever and tend to reveal less than good UI-level testing would. The payback on design reviews, meanwhile, is tremendous. Our code is intended for desktop, non-critical use, so I asked my boss to consider whether it was worth spending so much time on examining built code, given our experience not getting much out of it. I'm wondering whether the Slashdot crowd's experience has been similar."
Code reviews have a lot of value in two ways completely independent of how good they are at catching errors. First, they are a way to enforce various stylistic guidelines on code that make future maintenance much easier. Secondly, they are a tool for spreading knowledge about the code around to other members of your development team.
They can also catch some errors that are very hard to catch in any other way. I recently worked on a project in which I found an error that would've caused code to only fail in a very limited and non-obvious set of circumstances. The thing is, somebody would've almost inevitably encountered those circumstances and the phantom and nearly unrepeatable bug reports resulting from this would likely have never been solved.
I fear code ever stepping off into the land of incorrect behavior as there are few corrective mechanisms that will cajole the errant program back into doing something sane. The longer it goes without abject failure, the more weirdly wrong it will be. Therefore I think any and all measures to keep it from ever going there and making sure it dies as quickly as possible if it does are useful.
Need a Python, C++, Unix, Linux develop
When we did code reviews at DEC, they were done with several people generally familiar with the code, required considerable advance prep by the reviewers, and went over code basically line by line. This was not done for all code; nobody had time or resources. However, when a piece of code was doing something weird, it was a pretty effective way to figure out what might be wrong, where the programmer had been having trouble finding a problem. The one area where it tended not to work well was where the software was a driver that talked to some piece of hardware, and the programmer was the only one who really knew what the hardware was doing. The fact that the reviewers didn't know that hardware made finding bugs most difficult...
Design reviews are useful to catch problems early on, particularly the selection of poor algorithms or data structures. However, in the the software shops I've worked nobody does any documentation, which makes it particularly difficult to do a design review. So, as you can imagine, I haven't got much experience with this area.
Code reviews, on the other hand, are more about auditing code to make sure that people are following the coding standards and policies. After all, if you've got coding standards, how are you supposed to tell if anybody is following them without reviewing the code? Once your coding standards have been institutionalized -- that is, most people have internalized them and following them -- then what's the point of a code review?
So your best bet, then, is to reserve code reviews for junior developers until the coding standards are internalized; and use design reviews for the architects.
Part of the PCI-DSS (Payment Card Industry Data Security Standards) security requirements is to conduct quarterly code reviews of applications that process credit card data, or put an application firewall on your network to monitor these applications.
Like most companies, we spent about 10 minutes working out how much time our developers would have to spend on this per quarter - and then we decided to drop $30K on the firewall.
I did a code review once at a previous job. It consisted of a bunch of people saying it looked good and one person giving wrong advice. I later found a bug in the code that everyone had missed. One person did comment afterwards that he learned a new trick reading my code.
I think a code review at hire time, as part of the interview process, might be good... hell mandatory. I certainly wouldn't have hired the other developers in my group given the quality of code they produce (they were hired before me by someone who wasn't particularly competent himself)
how many defects are discovered in the review versus how many make it out the door?
I don't think defects are the only metric. Code reviews can result in a cleaner codebase that's easier to understand. Everyone occasionally writes bad code. A reviewer might say, "I see that it works, but I don't like it..." and mention an alternative solution. A reviewer might suggest that something is non-obvious and that a comment is warranted. Code reviews aren't just for bugs, they are to get better code.
A cat can't teach a dog to bark.
then code reviews would be redundant.
Err... no. Testing is not a replacement for code reviews, which do a variety of things, including enforcing coding and commenting standards, act as sanity checks in implementation of design, etc. They also find the bugs that you never thought to design tests against.
Test driven development is a good way of capturing requirements in testing up-front, rather than leaving that as a downstream activity the way conventional testing is done. Doing test-driven development will not cause your test set to be any more thorough than a properly done V&V test set.
A while back on /. we had a story about a serious bug in a major product (can't remember what it was) and someone commented that "this seems like the kind of thing that test-driven development would have caught" as if the tests the developers would have thought of doing in a test-driven environment would have been any different than the tests developers would have thought of doing in an environment with sane down-stream testing. There is absolutely no reason to believe this.
Blasphemy is a human right. Blasphemophobia kills.
There are apparently a couple different kings of things that are both called "code reviews", which one are you talking about? There's also the issue that they're supposedly (as in, according to actual studies) pretty good, so maybe you could do them slightly differently and get much better (more in line with the study results) effects.
Formal reviews is only meaningful if you have an equally formal specification that is unlikely to change often or at all. A lot of heavy backend systems could benefit from that, but this isn't one of them they should definately stop. Of the lighter:
Over-the-shoulder One developer looks over the author's shoulder as the latter walks through the code.
Email pass-around Source code management system emails code to reviewers automatically after checkin is made.
Pair Programming Two authors develop code together at the same workstation, such is common in Extreme Programming.
Tool-assisted code review Authors and reviewers use specialized tools designed for peer code review.
First one nearly never leads to good code in my experience, unless you manage to get just the right mix of writing code and helpful conversation, it's way too easy to zone out, take over, turn it into a lecture or whatever. Second one sounds like SPAM, who reads those? Pair programming can work, but I'm not sure it's worth the overhead.
Tool assisted is definately my favorite. Clone and branch then make your changes and request that they be merged back. You have to say something sensible about what you're doing as a whole, at least two people will look at it, they can comment or reject it. Not according to guidelines or design or whatever? Fix and resubmit. That, together with design meetings I think is the way to go.
Live today, because you never know what tomorrow brings
Pair programming can work, but I'm not sure it's worth the overhead
By "overhead" I assume you mean the cost of two developers to write one piece of code. In my experience, Pair programmers are more than twice as productive as a single developer when you factor in all the errors and bugs prevented by having two sets of eyes on the same problem. Of course this only works when you have a pair that can work together, which can be hard to find in some environments.
The other advantage you get from pair programming is that you have two people familiar with the code, not just one. Thus if one leaves the company, or goes on vacation and problem needs to be fixed, you always have another person (at least until both of them leave the company).
Even if you do nothing else that XP advocates, pair programming can really be worth it.
If you need web hosting, you could do worse than here
Are code reviews useful?
Lets see. Right click -> View source this web page. In the first 10 lines I see a variable called maybe with no comments as to what it means.
Yes, code reviews are useful.
Good developers paired with over-the-shoulder code reviews produce code that is just as good (or better), and is far more productive
My experience differs. Most people that say they're "good developers" aren't. What you really mean is "People that hate having someone watch them don't do well in pairs", and that's true.
If you need web hosting, you could do worse than here