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."
then you're doing it wrong. Plain and simple.
Code reviews shouldn't be a "full stop, let's go over this" event. Code review should be a part of the every day workings of the development team. Nothing should go into version control's master/trunk/HEAD until it has been reviewed.
Sometimes you'll find that stopping to review a single module is helpful, but most of the time it's actively harmful to the team, since it takes developer's concentration off of what they're currently working on to review things that they half-don't-remember, which then makes the code review take forever.
Review and document inline with your coding, and you'll find you'll never need a "Full Stop" review.
"Victory means exit strategy, and it's important for the President to explain to us what the exit strategy is." G.W.Bush
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...
As someone else noted, badly-run code reviews aren't worth much, if anything.
There was a lot written about code reviewing in the late 80's and early 90's that makes sense. If a review is conducted as a lesson in coding to others, nobody is going to get much out of it. If it is done as a last-ditch design review, that probably isn't going to work out well either.
If the staff is all people with lots of experience, it may not be that valuable. Alternatively, I see it as an extremely powerful tool for a staff that works mostly independently to come back together periodically and make sure everyone is on the same page. Especially when some team members have less experience.
Trying to bog it down with formality is pointless. But the early guidelines about "egoless" are right on target.
Where I work we do code reviews and they are definitly worth the time. 60% of the time the review doesn't flag anything. But by having anouther coder look at the code you can find those points when a comment would be very usefull, where an algorythm might break down to simple typo's , complexity issues and general readability
Code reviews are as much about code maintainability and ensuring the code follows standards then finding bugs.
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.
A code review is unlikely to uncover many errors. Most code is just too complex for another developer to spot errors. Unit testing is much better at that. What a code review can do is
I often don't like the choices people make, but I like the fact that people make choices. That's why I'm a conservative.
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
Exactly. Its also a great way to mentor newer developers and teach them the tricks of the trade. There is always more than one way to do something, but often they vary greatly in their performance and readability.
Well.. maybe. Or Maybe not. But Definitely not sort of.
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.
Quite simply, code reviews cost money and production bugs cost money.
We do code reviews for anything where it'll either be devastating expensive if we encounter a failure or if it'll be very hard to detect a failure. Otherwise, in my particular line of work, it's more economical to accept a lower cost and faster pace of development at the expense of dealing with a few bugs that are discovered in production.
But the company that gives you that paycheck does, and that's all that really matters. :)
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
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.
In my experience, pair programmers are only more efficient than each programmer working on their own when both programmers are bad.
Bonus: Most development managers that like pair programming have hiring practices that find the worst programmers (but they're generally fairly well dressed and always show up to meetings on time).
Good developers paired with over-the-shoulder code reviews produce code that is just as good (or better), and is far more productive.
If it's ever not more economic to do code reviews, then I respectfully submit that You're Doing Them Wrong(TM).
The improvements in the general standard of code, and consequently its maintainability, should easily outweigh the modest time spent on reviews. Likewise, the efficiency benefits just from sharing basic awareness of how various systems work and useful coding techniques around a development group should be enough to justify the time spent. And those are both without allowing for any actual bugs that would have been observed by your customers but got fixed much earlier and cheaper because the review caught them.
Incidentally, if you don't think it's worth getting even a quick glance from a second pair of eyes on even a small bug fix, you should look up the research on how many bugs originate from a one- or two-line change to the code. It's a staggeringly high proportion.
Now, a lot of places have tried full, Fagan-style, heavyweight reviews, and yeah, those pretty much suck for most software development groups. But that doesn't mean you can't employ a lighter process with the same goals. With the kinds of tools available to co-ordinate reviews and annotate code these days, your overheads should be near zero and you can do a lot of the work on-line rather than shoving everyone into a room for a few relatively unproductive hours.
If you disagree, post your argument. (-1, Overrated) isn't your personal censorship tool for views you don't like.
Actually, it's reasonable to periodically review and improve and streamline all of your processes. Any part of the process that takes time or money should be justified by an improvement in the metrics after adding that part of the process. if the metrics don't improve after adding that part, then that part should be removed from the process. This can help lead to less busywork and less paperwork, rather than more, as it sounds at first blush.