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."
Having worked on life critical type systems where every line of code was reviewed before making it into the product, I have to say that I've seen them add a lot of value when done properly.
They also cost a lot.
The first question I would ask in your situation is: are you doing them right?
Do bugs get discovered later after deployment? Are the bugs in areas of the code that were supposedly reviewed? If so, you might be doing it wrong.
And as much as we _hate_ the word... I have to say it...
METRICS!
If you truly want to make a decision on whether code reviews are worth it.. you need to know:
- how much does it cost to conduct the reviews
- how many defects are discovered in the review versus how many make it out the door (in other words, how good are you at it)
- how how much more does it cost you when a bug gets discovered after deployment? In a life critical system, it costs a fucktonne.. in a desktop app.. it may not be that bad.
Once you know these, the picture should be clear
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
What we've found is that code reviews take forever...
Ugh. Are you reviewing each individual commit (where code reviews are quick and very effective), or are you rounding up a bunch of developers in a conference room and reviewing an entire module using an overhead projector?
Peer-to-peer reviews of individual diffs using good workflow tools have been very effective at several places I have worked and in open-source projects to which I have committed.
Some of the fastest team development velocity I have experienced has been with peer code reviews within the team.
A good style guide also helps...
Yesterday it worked; today it is not working; Windows is like that...
Have each developer line up with his/her/its code printed out on a cue card board and stand in a line up. Execute the least likely to make it to the compiler, then you will "motivate" the rest to write better code..
Even the best programmers make mistakes. Having another set of eyes is invaluable for detecting bugs before they become problems. Having to explain in words the rationale for a design decision often helps you better understand your own design, and to see potential problems with it. Sometimes you come up with something better on the spot. Also, if you get hit by a bus, your fellow programmers can take over without having to reverse-engineer your thoughts. Please, more code reviews.
Well, my last workplace had code reviews for everything, and I found them tremendously helpful. They accomplish a few things:
Furthermore, if I edit code that was written by (or is owned by) Bill, I'll ask him to review it so he'll know about the new feature I added (which is good, if he ends up having to support it).
A cat can't teach a dog to bark.
You often *have* to review a entry level programmer's work until it reaches an acceptable quality. I consider code reviews as a method of improving the programmer more so than the code. One an engineer is producing generally acceptable code it becomes safe enough to treat their code as a black box and wait for problems to be unearthed by testing. If you are shipping bugs your problem is testing, not code reviews. Finally, the cheapest way to do code reviews is for a manager to just scan submitted code from time to time and send out polite emails if they see something amiss. On the other hand getting five senior guys in a room to discuss the work of another senior engineer is a just going to result in unproductive, cranky engineers.
-- http://thegirlorthecar.com funny dating game for guys
My working life has been spent in projects developed by individuals or small teams (less than six programmers). I would describe my working environments as "CMM level 1 and damn proud of it." The teams I have been on have been consistently successful without having a consistent methodology. The success is the result of having a bunch of competent people who respect each other, and are motivated by the idea of producing something that customers will pay money for, because it works. And by the knowledge that they'll stick with the project for a while, and if they make any messes they'll be the same people who have to clean them up later.
I've suffered throughout my working life from inexperienced managers and programmers who suffer from what I call delusions of grandeur. 90% of the stuff that people learn about project management seems to me to be intended for use in projects with fifty programmers or more. Projects where the manager can't get a grasp of the project by walking around and schmoozing with people. Projects that are big enough that there are constantly people leaving and joining them. I don't know about projects like that. I'm inclined to think that formal management processes may be useful, even essential there.
But on small-team projects, it just gets in the way.
The problem is that the books that explain the capital-M Methodologies and the code review process and so forth never say, in so many words, that these are management procedures for projects with more than fifty people. They just say, "this is how you do it." And people come out of courses believing that "doing it right" means applying all this stuff. And that anything else is unprofessional.
The hidden assumption is that everyone wants to follow a career path where they manage more and more and more people on bigger and bigger and bigger projects and make more and more and more money. When I worked at a Fortune 500 company, people were very frank about it. It was a waste of time proposing or working on small stuff. You had to "think big" or management would never take any interest in what you were doing.
Well, I'm here to say that if you want to think big and manage like the big boys do, fine. But don't try it on a small-team project where the team members have gelled into a coherent unit, and know each other and work well together, and plan to stick around for a while.
Code reviews? Gimme a break. In the natural course of events, other team members are going to have to work with my code. If I don't care about what they think about it _when they're editing it to get their job done,_ I'm sure not going to care what they think about it in some room with a whiteboard. And we're effectively eviewing each others' code in the natural course of getting our jobs done, then sealing ourselves into a room with a whiteboard and no debugger, isn't going to be any better than what we naturally do without any formal process.
If you are shipping bugs your problem is testing, not code reviews.
No, no, no. If you are shipping bugs the problem is that the bugs are introduced in the first place. Blaming testing is not getting at the root cause of the problem. You cannot test quality into a product. I've worked on products where tons of bugs were shipped and people wanted to blame the test group, but the fact is that the test group wrote tons of bug reports that no one had time to look at much less fix. Pretty much every bug report that came from the field had already been found internally. When testing started, the product was already so crappy that a few bug fixes just were not enough.
As a CS person, I view the Software Engineering research discipline with quite a bit of skepticism. But one thing that the research is pretty clear on is that code reviews do work. If the submitter is not seeing that, then he is either on the verge of a major research breakthrough that invalidates decades of SE research or he is doing them wrong.
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
Pair programming is the most effective in the circumstance that makes the best use of it - two circumstances to be exact :
1. Pair an experienced programmer with an inexperienced programmer.
2. Pair an experienced programmer with strong subject matter expertise on one domain with an experienced programmer with a completely different domain of experience.
The first is highly effective in getting the weaker guy up to speed on the first guy's domain. The second is effective at solving a set of problems that eclipse the domain of either developer.
Glonoinha the MebiByte Slayer
I'll add a few more.. no, no, no, no, no...
Your argument is valid but does not actually counter the original statement. The testing team apparently did their job and found the bugs in the system. Yes, it would be nice if the original coders hadn't done such a great job of creating these bugs BUT this is not the problem (and in reality tends to not be feasible at least in total.. you can minimize bugs but rarely if ever eliminate their creation)
The problem is that the testing wasn't utilized! If the sole job of a testing team is to submit bug reports that can be ignored then why have your testing team? Save the money and just ship the first raw release. This product should have never made it out the door until those bug reports were resolved in some fashion (even if that "resolution" is marking the bug as "release acceptable, fixed in next version/patch"). In this case I would say it is the release team OR probably the management's fault that these bugs made it to the consumer. They paid a lot of money on a testing team and then ignored their feedback and cost themselves even more in customer dissatisfaction and support calls for bugs *they already knew about*.
Wasteful.
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.