Posted
by
kdawson
on Saturday June 13 2009, @04:23PM
from the bang-for-buck dept.
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.
it's a pretty easy thing to track.. most shops already have a bug tracking system.. you just need to add in a way to track how much stuff gets discovered in code reviews.. then have some intern hack out a spreadsheet in leu of getting valuable job experience.
So what you're saying is we have to have a review of the reviews...
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.
Sadly every security vulnerability on the products I've worked on were found after shipping in code that was reviewed (and not only that - sometimes very obvious bugs - like treating strings as fixed values, and not checking or sanitizing inputs).
So I guess they either have to be done right, or they aren't all that useful.
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.
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.
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.
Where I work we have code reviews at various degrees. I find someone else just reading over my code and emailing suggestions helps tons; It spots obvious errors, ways code could be done better, and just a ton of other things. It helps improves my own coding style too.
Code Reviews are useful when they are done right. But before you start using code reviews you should introduce automated static analysis of the code during the builds. A lot of crap can be discovered by static analysis. This saves you a lot of effort on the tedious parts of code reviews.
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.
Small projects in the stage of trying things is roughly the definition of drafting a design.
And quite frankly, if you don't draft the design correctly from the beginning, it can cause huge problems later. Some projects can skip this step and do iterative design (most open source projects, software libraries, e.g.), but commercial products typically don't have the luxury of having a product delayed by a huge amount because someone's design was wrong and it is now a burden to remove and replace.
Not stopping to design for the future early on is the reason Xorg is a complete and absolute clusterfuck today, for example.
by Anonymous Coward
on Saturday June 13 2009, @04:36PM (#28322605)
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...
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.
by Anonymous Coward
on Saturday June 13 2009, @04:38PM (#28322623)
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..
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.
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.
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.
by Anonymous Coward
on Saturday June 13 2009, @04:42PM (#28322665)
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.
Well, my last workplace had code reviews for everything, and I found them tremendously helpful. They accomplish a few things:
catch basic errors (second set of eyes)
get new people up to speed (e.g., a more experienced dev says "actually, we have a library that would help here..."). Also, reviews can help an inexperienced engineer become a better developer.
keep employees abreast of new development (at least two people know about every commit in detail)
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).
The value of the code review depends on several factors, the most important being the coding standard against which the code is being reviewed. If the coding standard has a lot of hard and fast rules about what goes into the comment block, where variables should be declared, whether brackets go at the end of a line or on their own line, and how many returns a method can have, then the code review will be mostly about religious issues and petty formatting. On the other hand, a coding standard with many "should"s instead of "shall"s that allow the developer, combined with reviewers and especially review moderators who know what is important can what isn't, can make code reviews very useful, especially early in a project and especially with junior developers.
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
1. Coach new developers by helping them learn and/or remember best practices: "Please use "literal".equals(variable) rather than variable.equals("literal"), just in case the variable is null."
2. Remind people to follow the important standards, or recognize that you're missing important standards and need to set one: If your DAO "find" method doesn't find the expected record, do you return null or do you throw an exception? Both have strengths, but everyone on the projct should be doing it the same way. Code reviews will help uncover discrepancies.
3. Uncover future maintenance issues. The code may be too complex for reviewers to find bugs during the review, but they should at least be able to follow what the code is doing. If they can't, the code either needs restructuring or better commenting.
by Anonymous Coward
on Saturday June 13 2009, @05:08PM (#28322835)
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.
What we've found is that code reviews take forever...
In one place where I worked in the past, we had a very simple rule: if you are doing a code review, and it takes longer than 10 minutes, then you stop it right there and return the whole thing marked as "overcomplicated" - if it really takes that long, then either the code is written in non-obvious ways and/or poorly commented (which will result in poor maintainability anyway), or the change is too big for one source control commit. By and large, it worked, even if you have to make exceptions occasionally (but at that point you know it's not a typical review, and pay more attention).
In addition to that, you might want to consider better tooling. If you're doing reviews by sending.diff files over email, you're doing it wrong - there are many specialized tools out there that will do automatic and smart diffing (including between rounds in a multi-round CR), notify people responsible for affected files, allow to set up the workflow according to your needs, enable attaching review comments and conversations to particular files and lines of code, and so on. The shop I was working for used Code Collaborator [smartbear.com] , and I found it to be pretty good, but there are plenty other similar tools out there, and you might even be able to find some good free ones.
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.
The primary effect of code reviews has nothing to do with finding problems during the review itself. It improves quality before the code ever gets to review, because people care far more about what they do in the first place if they know there is even a chance others will see and criticize them later for doing it wrong.
This is why stores put up fake security cameras. The notion that they have someone sitting there watching the camera continuously is ridiculous, yet a camera has a huge effect on people's tendency to commit crimes nonetheless.
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.
More details on what your version of a "code review" and a "design review" are would probably get better answers...
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.
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.
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.
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.
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".
I've had numerous 4-6 hour pair programming sessions that churned out more well written, largely bug-free software than most people write in a week.
The problem is, when pairs are mandated, you end up with a situation where you write code for 5 minutes, then one guy gets up to go to the bathroom for 15 minutes (the other guy surfs the web waiting for him to get back). Then they write more code for 5 minutes and the other guy goes to get a can of Dew, stops and chats with someone, etc.. comes back and they write 5 minutes of code and then the first guy goes to lunch... etc..
You need two people that are a team, working together, who want to be there and doing it. Otherwise it's just pointless. In most cases, I like to find an unused conference room or office to allow us to be completely "in the zone", and we generally don't come out until we're done for the day.
Occasionally you get into a disagreement about how to go about things, and that wastes time. But more often than not, a pair that's "in sync" churn out amazing amounts of good code.
The fact that you talk about mailing each other patches seems to indicate to me that you're talking about a maintenance situation. Pair programming does not work well for small patch change situations, or rather it's a waste of time.
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.
Also, having someone watch over you makes it harder to slack off on Slashdot. Harder, but not impossible, of course.
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, unicorns are 2.7 times better than mermaids at debugging.
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.
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 m
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.
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.
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*.
Why do you think I live in a fantasy world? I'm just talking about the root of the problem. The root of the problem is that writing software is hard and programmers are human and make mistakes and those mistakes result in bugs. If you can reduce the rate at which bugs are introduced, you gain a huge increase in quality and a huge reduction of cost because fixing bugs after they are introduced is very expensive.
It is a fact that there are known practices that result in better code in the first place and code reviews are one of them. It is also a fact that many companies do not use these practices and hope that the testing team can pick up the slack.
So the management question is where to spend resources to improve quality and meet deadlines. Do you just add more and more testers so you can find the bugs faster and faster? Do you throw more coders at the problem so you can get the crappy code to the testers earlier? Do you adopt some of the practices that are known to work like reviewing code even though it means some of your coders will have to spend their time not actually writing code?
Since most companies continue to do stupid things that don't work, you tell me who lives in a fantasy world.
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.
This is the most reasoned response. I would think that, like validation, code review activities should be appropriate to the level of risk involved. I also believe that good reviews, be it design, code, or documentation, should be kept on the topic of acceptability, not perfection. Code can very easily suffer from the word-smithing problem. If you start talking about a problem in code that really does not have a significant impact on quality, it's time to move to the next item. This is easier said than
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.
Stupid question, but doesn't that miss things which are technically OK, but likely to lead to problems down the road?
Things like poor naming conventions, improperly formatted or under documented code.
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.
Synergy, leverage, low hanging fruit, etc.. (Score:5, Informative)
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
Re:Synergy, leverage, low hanging fruit, etc.. (Score:5, Funny)
If you truly want to make a decision on whether code reviews are worth it.. you need to know[...]
So what you're saying is we have to have a review of the reviews...we're going to be here a while aren't we?
Parent
Re: (Score:3, Insightful)
Oh calm down..
it's a pretty easy thing to track.. most shops already have a bug tracking system.. you just need to add in a way to track how much stuff gets discovered in code reviews.. then have some intern hack out a spreadsheet in leu of getting valuable job experience.
Re: (Score:3, Funny)
Well, in that case, we should do a thorougly review of the reviews of the reviewed review reviews.
BUFFER OVERRIDE
Re:Synergy, leverage, low hanging fruit, etc.. (Score:5, Insightful)
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.
Parent
Re:Synergy, leverage, low hanging fruit, etc.. (Score:4, Funny)
Parent
Re: (Score:3, Informative)
Sadly every security vulnerability on the products I've worked on were found after shipping in code that was reviewed (and not only that - sometimes very obvious bugs - like treating strings as fixed values, and not checking or sanitizing inputs).
So I guess they either have to be done right, or they aren't all that useful.
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.
Parent
Re:Synergy, leverage, low hanging fruit, etc.. (Score:5, Insightful)
Parent
a fucktonne? (Score:5, Funny)
Damn europeans and their metric system.
How many shitloads are in a fucktonne?
Parent
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.
In a word, yes (Score:3, Insightful)
When done right (Score:5, Informative)
Code Reviews are useful when they are done right. But before you start using code reviews you should introduce automated static analysis of the code during the builds. A lot of crap can be discovered by static analysis. This saves you a lot of effort on the tedious parts of code reviews.
If you're code review is taking forever... (Score:5, Insightful)
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.
Re:If you're code review is taking forever... (Score:5, Insightful)
And quite frankly, if you don't draft the design correctly from the beginning, it can cause huge problems later. Some projects can skip this step and do iterative design (most open source projects, software libraries, e.g.), but commercial products typically don't have the luxury of having a product delayed by a huge amount because someone's design was wrong and it is now a burden to remove and replace.
Not stopping to design for the future early on is the reason Xorg is a complete and absolute clusterfuck today, for example.
Parent
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...
U R DOIN IT WRONG :) (Score:5, Insightful)
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...
Line them up (Score:5, Funny)
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..
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.
Yes, yes, and then some (Score:5, Informative)
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.
Re: (Score:3, Insightful)
Also, if you get hit by a bus, your fellow programmers can take over without having to reverse-engineer your thoughts.
But if I get hit by a bus, I won't care whether my fellow programmers can take over without having to reverse-engineer my thoughts.
Re:Yes, yes, and then some (Score:5, Insightful)
But the company that gives you that paycheck does, and that's all that really matters. :)
Parent
Depends... (Score:5, Insightful)
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.
They are worth it (Score:5, Insightful)
Code reviews are as much about code maintainability and ensuring the code follows standards then finding bugs.
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.
yes! (Score:5, Insightful)
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).
Coding standards (Score:5, Insightful)
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
Delusions of grandeur (Score:5, Insightful)
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.
"Take forever" (Score:4, Insightful)
What we've found is that code reviews take forever ...
In one place where I worked in the past, we had a very simple rule: if you are doing a code review, and it takes longer than 10 minutes, then you stop it right there and return the whole thing marked as "overcomplicated" - if it really takes that long, then either the code is written in non-obvious ways and/or poorly commented (which will result in poor maintainability anyway), or the change is too big for one source control commit. By and large, it worked, even if you have to make exceptions occasionally (but at that point you know it's not a typical review, and pay more attention).
In addition to that, you might want to consider better tooling. If you're doing reviews by sending .diff files over email, you're doing it wrong - there are many specialized tools out there that will do automatic and smart diffing (including between rounds in a multi-round CR), notify people responsible for affected files, allow to set up the workflow according to your needs, enable attaching review comments and conversations to particular files and lines of code, and so on. The shop I was working for used Code Collaborator [smartbear.com] , and I found it to be pretty good, but there are plenty other similar tools out there, and you might even be able to find some good free ones.
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.
Security Camera Effect (Score:4, Insightful)
The primary effect of code reviews has nothing to do with finding problems during the review itself. It improves quality before the code ever gets to review, because people care far more about what they do in the first place if they know there is even a chance others will see and criticize them later for doing it wrong.
This is why stores put up fake security cameras. The notion that they have someone sitting there watching the camera continuously is ridiculous, yet a camera has a huge effect on people's tendency to commit crimes nonetheless.
Re:Are they worth it? (Score:5, Informative)
http://en.wikipedia.org/wiki/Code_review [wikipedia.org]
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.
More details on what your version of a "code review" and a "design review" are would probably get better answers...
Parent
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.
Parent
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.
Parent
Re:Are they worth it? (Score:5, Insightful)
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.
Parent
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.
Parent
Re:Are they worth it? (Score:4, Insightful)
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".
I've had numerous 4-6 hour pair programming sessions that churned out more well written, largely bug-free software than most people write in a week.
The problem is, when pairs are mandated, you end up with a situation where you write code for 5 minutes, then one guy gets up to go to the bathroom for 15 minutes (the other guy surfs the web waiting for him to get back). Then they write more code for 5 minutes and the other guy goes to get a can of Dew, stops and chats with someone, etc.. comes back and they write 5 minutes of code and then the first guy goes to lunch... etc..
You need two people that are a team, working together, who want to be there and doing it. Otherwise it's just pointless. In most cases, I like to find an unused conference room or office to allow us to be completely "in the zone", and we generally don't come out until we're done for the day.
Occasionally you get into a disagreement about how to go about things, and that wastes time. But more often than not, a pair that's "in sync" churn out amazing amounts of good code.
The fact that you talk about mailing each other patches seems to indicate to me that you're talking about a maintenance situation. Pair programming does not work well for small patch change situations, or rather it's a waste of time.
Parent
Re:Are they worth it? (Score:4, Insightful)
Also, having someone watch over you makes it harder to slack off on Slashdot. Harder, but not impossible, of course.
Parent
Re:Are they worth it? (Score:5, Funny)
In my experience, unicorns are 2.7 times better than mermaids at debugging.
Parent
Re:Are they worth it? (Score:5, Insightful)
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.
Parent
Re: (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 m
they are worth it (Score:5, Insightful)
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.
Parent
Re:they are worth it (Score:5, Insightful)
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.
Parent
Re:they are worth it (Score:5, Insightful)
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.
Parent
Re:they are worth it (Score:4, Insightful)
Why do you think I live in a fantasy world? I'm just talking about the root of the problem. The root of the problem is that writing software is hard and programmers are human and make mistakes and those mistakes result in bugs. If you can reduce the rate at which bugs are introduced, you gain a huge increase in quality and a huge reduction of cost because fixing bugs after they are introduced is very expensive.
It is a fact that there are known practices that result in better code in the first place and code reviews are one of them. It is also a fact that many companies do not use these practices and hope that the testing team can pick up the slack.
So the management question is where to spend resources to improve quality and meet deadlines. Do you just add more and more testers so you can find the bugs faster and faster? Do you throw more coders at the problem so you can get the crappy code to the testers earlier? Do you adopt some of the practices that are known to work like reviewing code even though it means some of your coders will have to spend their time not actually writing code?
Since most companies continue to do stupid things that don't work, you tell me who lives in a fantasy world.
Parent
Depends what you are reviewing (Score:5, Insightful)
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.
Parent
Re: (Score:3, Interesting)
Not much, it doesn't (Score:5, Insightful)
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.
Parent
Re: (Score:3, Insightful)
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.
Parent