Are Code Reviews Worth It? 345
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."
Yes! (Score:5, Interesting)
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.
They sometimes find really difficult problems (Score:3, Interesting)
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...
Depends on what you're looking for (Score:5, Interesting)
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.
PCI-DSS Code Reviews (Score:3, Interesting)
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.
If you did test-driven development (Score:2, Interesting)
Re:Yes! (Score:2, Interesting)
Re:Are they worth it? (Score:3, Interesting)
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)
Re:Synergy, leverage, low hanging fruit, etc.. (Score:5, Interesting)
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.
Re:If you did test-driven development (Score:4, Interesting)
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.
Re:Depends what you are reviewing (Score:3, Interesting)
Code Reviews for the lulz (Score:3, Interesting)
Oh the dilemma. Do I take ownership and defend "my" code, or say nothing and enjoy watching him squirm? I did the latter watching him go "ummmmm ummmmm ummmmm". Finally medicore programmer admitted he copied it and it was really mine. I said "Well it was working when you copied it off me."
Needless to say he ended up being promoted to manager. Not even a PHB which requires a bit of cunning, he was just a flat out idiot. I keep in touch with some of my coworkers to this day and whenever we want to describe a certain type of idiot we use his name. Duuuuuuuuuuuuuuh.
Re:If you did test-driven development (Score:3, Interesting)
Second, modern tests are documentation. For example, if you were using rspec for tests, you can do (this is pseudocode):
Method "MyMethod" should do {this}.
Method "MyMethod" should do (that).
Method "MyMethod" should not do (something else).
Method "AnotherMethod" should do (yet another thing).
This is very straightforward, and does not tend to "lead to other problems down the road".
Having said that, these kind of tests are for code, and often not for overall user experience. If you want to test your interface, you might want to use other tools. And nothing replaces a good QA person or team to test the final product.
Re:Are they worth it? (Score:5, Interesting)
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.
Re:Synergy, leverage, low hanging fruit, etc.. (Score:1, Interesting)
I can say in our company the benefits are multi-fold.
Programmers skill levels vary. And also vary by subject. If you are going to allow someone to code the multi-threaded part of your project how do you insure that they've protected the write code the write way - which thread synchronization element did they use where? Also remember the uproar about the code SGI put into Linux that SCO was bantering about. I believe it was simple memory allocation. Stuff that already existed elsewhere - duplicated uselessly. Does everyone on your team know how to optimize C++ STL?
Then there is maintainability. Lets forget outsourcing for a moment. I'm currently rewriting a large chunk of code - written in C#. If you think Java had a tough time in the early years because of junk 'web' programmers, you haven't seen anything yet. Everyone and their mother thinks they can code in C#. In my case the implications of using events, poor locking strategy etc. gave me the opportunity of 7x performance increase. Yes, 700%. Apparently waiting for the GUI to update isn't good for a background thread. They did things like send data across a network, start a thread to wait for response, then send an event to tell another thread that data had arrived. From a design review I'm sure it looked OK. "Hey we are doing asynch communication!!!!"
I know I hate it, but what about variable names. Who checks if the comments make sense?
As the developers get better code reviews become less fruitful. One of the issues I have now is that most sleep through the code review. I don't mean literally, but I showed my code, everyone agreed it was good, only to have me go back to my desk and realize it would crash on one of the exception cases. Hopefully testing would have caught it - we'll never know.
So... what we do for code review is a 1 on 1 session. It is the developers job to convince the mentor that he, the developer, is doing the job right. It is then the mentor who makes the final submission (he buys the donuts if the build breaks :) ) The idea is to keep from rubber stamping.
So defect catching is one part. Performance another, and maintainability yet another.
BTW, before my changes to the code, everyone thought our competitors product was better/faster. Now we are about 20-30% ahead of them. So what did that cost our customer base?
Of course now everyone in the company thinks I'm god (my head barely gets through the door as it is). If only we did this code review before we shipped.
Re:Are they worth it? (Score:5, Interesting)
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.
Re:Yes! (Score:3, Interesting)
I just want to highlight your second point. I believe that THE most important thing gained from code reviews is the spreading knowledge and gaining understanding. New development is always great, but most programming is maintaining/fixing/improving existing projects. A code review is a great way to really learn about code readability. You actually get to see other people read your code and you get to read other people's code. All of this code is fresh in someone's mind so it can be explained, and how to make it more readable can be discussed. I learned a ton about writing maintainable code at my first job where we did regular code reviews.
On the more technical side, often once the code is discussed much simpler ways to solve the problem is discovered. It isn't about the individual bug fixes/improvements that can come from a code review. Its really a way to improve your programmers.
WTF does maybe mean? (Score:5, Interesting)
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.
Re:Are they worth it? (Score:1, Interesting)
I agree. I kind of broke up the practice of pair-programming at my last job. Everyone on the team found that they were more productive if they were just let alone, had people to talk to if they had an actual problem, and just emailed each other their patches for "review". We would still pair for issues in multiple domains, but many times, a programmer and a chat client or the occasional "Hey, where does ... live?" is great.
On the other hand, it's kind of nice to not have to type while you think.
Re:they are worth it (Score:3, Interesting)
Just because it's fantasy (and it is) doesn't mean that's not the best/cheapest way to quality. The cost of fixing something in QA vs the original development is significant. Depending on who you believe (which study), it can be 2-10 times the cost. We'll be pessimistic(ish) and say 3x. That's basically enough to pay for pair programming, actually, which probably would be as cost-effective at producing quality as having the QA team. A bit of usability testing is about all that's left that developers are no good at (you've spent months focused on the problem, of COURSE it looks intuitive to you!).
Not that we do this at $work, of course. That's crazy talk.
/me sighs
Re:Are they worth it? (Score:3, Interesting)
It also depends on the personalities. Some programmers are too egocentric to be useful in pair programming. They aren't too good in other contexts either though.
Re:Are they worth it? (Score:3, Interesting)
Except that the company pays the opportunity cost of having you mentoring instead of coding, as you're on the clock either way.
Unless of course he wants to have his cake and eat it to and have you programming and teaching simultanously.
Re:they are worth it (Score:3, Interesting)
I don't disagree at all. But my experience is that pretty much everything depends on the attitude of the overall organization. I worked at a company where we developed individually, with some consulting back and forth. We'd pull people in to try to diagnose problems, but generally not what you would call pair programming or code reviews. The product was solid... not 100% bug free, but certainly a quality product that was enhanced over many years. That was the result of an organizational attitude that we were going to ship a quality product.
Eventually we got bought out by a "we are hot java programmers, unit testing rah rah rah" company, and they shipped utter crap. They always had the attitude that "hey it has to ship, we'll just stay up late and jam all nite and ship the disk, and by the time they load the disk, we'll have a fix version". I was actually on a call where people were laughing about how the contract said we had to ship a disk by a particular date, but didn't say it had to work. No technique on the planet could have saved their product.
Re:Are they worth it? (Score:3, Interesting)
Good developers paired with over-the-shoulder code reviews produce code that is just as good (or better), and is far more productive.
This I agree with wholeheartedly - a relatively quick over-the-shoulder review before a checkin provides the advantage of the extra eyeballs in pair programming/code reviews without the ridiculous disadvantages of redundancy and annoying meetings. Though as you said, it does assume good developers...
Re:Are they worth it? (Score:4, Interesting)
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.