Slashdot is powered by your submissions, so send in your scoop

 



Forgot your password?
typodupeerror
×

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."
This discussion has been archived. No new comments can be posted.

Are Code Reviews Worth It?

Comments Filter:
  • In a word, yes (Score:3, Insightful)

    by iamdjsamba ( 1024979 ) on Saturday June 13, 2009 @05:30PM (#28322551) Homepage
    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.
  • by ciroknight ( 601098 ) on Saturday June 13, 2009 @05:36PM (#28322603)
    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.
  • by RealTime ( 3392 ) on Saturday June 13, 2009 @05:37PM (#28322613)

    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...

  • by Anrego ( 830717 ) * on Saturday June 13, 2009 @05:38PM (#28322617)

    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.

  • Depends... (Score:5, Insightful)

    by cdrguru ( 88047 ) on Saturday June 13, 2009 @05:41PM (#28322651) Homepage

    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)

    by Stevecrox ( 962208 ) on Saturday June 13, 2009 @05:42PM (#28322659) Journal
    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.
  • yes! (Score:5, Insightful)

    by piojo ( 995934 ) on Saturday June 13, 2009 @05:47PM (#28322703)

    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).

  • Coding standards (Score:5, Insightful)

    by readin ( 838620 ) on Saturday June 13, 2009 @05:51PM (#28322743)
    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.
  • they are worth it (Score:5, Insightful)

    by acidrain ( 35064 ) on Saturday June 13, 2009 @05:54PM (#28322751)

    no

    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.

  • by hedwards ( 940851 ) on Saturday June 13, 2009 @05:56PM (#28322767)
    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.
  • by Bill, Shooter of Bul ( 629286 ) on Saturday June 13, 2009 @06:04PM (#28322811) Journal
    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.
  • by Anonymous Coward on Saturday June 13, 2009 @06: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.

  • by grahamsz ( 150076 ) on Saturday June 13, 2009 @06:08PM (#28322837) Homepage Journal

    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.

  • by readin ( 838620 ) on Saturday June 13, 2009 @06:13PM (#28322861)

    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.

  • by SixAndFiftyThree ( 1020048 ) on Saturday June 13, 2009 @06:16PM (#28322883)

    If I'm code review is taking forever, it's because I haven't documented I'm code. So in a culture of constant code reviews, I remember to document. Adding comments no longer wastes time; it *saves* time in the pretty near future. I also write fewer lazy hacks than I used to, because of the fear of having to explain them. And my code gets better for other reasons mentioned on this thread.

    The OP didn't explicitly say that they were reviewing code months after it had been written, but no, archaeology and programming don't mix any too well. OTOH in a company where Hitting The Release Date is all-important, there is certainly a temptation to postpone until after that Date everything that can possibly be postponed, and pointy-haired types do think code review (and documentation) can be postponed.

    What would you say to a teenage boy who thought putting on the condom could be postponed until after the ... release?

  • "Take forever" (Score:4, Insightful)

    by shutdown -p now ( 807394 ) on Saturday June 13, 2009 @06:16PM (#28322885) Journal

    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.

  • by paazin ( 719486 ) on Saturday June 13, 2009 @06:28PM (#28322951)

    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.

    But the company that gives you that paycheck does, and that's all that really matters. :)

  • by cetialphav ( 246516 ) on Saturday June 13, 2009 @07:01PM (#28323131)

    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.

  • by Glonoinha ( 587375 ) on Saturday June 13, 2009 @07:13PM (#28323197) Journal

    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.

  • Dijkstra's troll (Score:1, Insightful)

    by Anonymous Coward on Saturday June 13, 2009 @07:13PM (#28323199)

    The purpose of the few coding reviews I have assisted was only to improve code metrics in order to avoid:
    - too much nesting
    - too long procedure or files
    - languages syntaxes that are rarely the best one (operator ?, struct in C++, ...)
    The modified code has far better metrics, but has exactly the same number of bugs and is less readable because of the late cueless refactoring.

    I hate the article of Dijkstra about the Goto, because of these practices that degenerated from it.
    Computer programing should be an art form like poetry and music (D. Knuth) and shall not be restricted to a subset. It is as stupid as refusing alterations in music or avoiding rare words in books.

  • by Matheus ( 586080 ) on Saturday June 13, 2009 @07:20PM (#28323233) Homepage

    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.

  • by cetialphav ( 246516 ) on Saturday June 13, 2009 @07:26PM (#28323263)

    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?

    The great thing is that none of these metrics are that hard to collect. You should already know the cost of production bugs because that is just a basic component of your business.

    To collect the cost to conduct reviews, you just need to ask everyone who does a review how long it took them. You can store this on paper or in a simple little database. Reviewers just need to remember to look at the clock when they do the review. Some computerized review systems can collect this for you automatically.

    To track the defects, you just need to track when someone says "You need to change this because ...". Assigning someone as a secretary in a meeting works well (assuming you have a meeting). Or you can just collect this in the emails or other online system that you use. The harder thing is to estimate the value of these found defects, but it isn't too hard to come up with some simple heuristics that serve as a reasonable approximation.

    Clearly, many organizations collect this in a way that is painful and intrusive and that gives these metrics a bad name. But it doesn't have to be that way. I have seen this done in simple ways that flow easily with the rest of the development process. Having this information is extremely powerful because without it everyone is just guessing and you become subject to the hunches of middle management because their guesses have more weight since they are the boss.

  • by ivan256 ( 17499 ) on Saturday June 13, 2009 @07:26PM (#28323265)

    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.

  • by linzeal ( 197905 ) on Saturday June 13, 2009 @07:27PM (#28323275) Journal

    Best code reviews I have seen cost a lot of money in man hours and are usually are done by an in house group that specializes in it. For the last firmware review (2048k ASM) we had 3 salaried guys making over 120k a year spend 3 months on it to find 4 critical bugs and dozens of lesser ones. Shipping that firmware would of made us liable for over 2 million dollars worth of hardware. So if you look at it another way ~1/20th of the cost of the product was invested in the final code review.

    Having the same group review their own code is like the fox guarding the hen house, he might be a vegetarian but I doubt it.

  • by DrLang21 ( 900992 ) on Saturday June 13, 2009 @07:34PM (#28323315)
    Running good code reviews are like running any good meeting. It's difficult and often requires special skills that most people just don't have. Keeping people on track and on topic is difficult. Especially with us technical types.
  • by cetialphav ( 246516 ) on Saturday June 13, 2009 @07:46PM (#28323395)

    What I am saying is that you cannot just throw some sloppy code together and expect to fix everything up in the test process. When you try to do that, you get an endless mountain of bugs. Since the code is so bad, each bug fix just causes additional bugs because new test cases get opened up (or because of unexpected code interactions). When you plan on 2-3 months of testing and it is 8 months later with no end in sight, you start to understand why people just hold their nose and release to customers.

    If you want a quality product delivered on time, you have to start by constructing the code with quality in mind and code reviews are an important part of that.

  • Re:Depends... (Score:3, Insightful)

    by Cyrano de Maniac ( 60961 ) on Saturday June 13, 2009 @08:09PM (#28323529)

    I pretty much agree with the parent.

    When I was fairly new to my company code reviews were reasonably helpful, as I was certainly not an expert in the areas I was fixing bugs, and there was a lot of undocumented knowledge of how various components interacted with eachother. As time went on and I became more proficient in these areas, code reviews began to be less useful.

    I then moved on to taking primary responsibility for an important system library whose original developer left years earlier, and the current maintainer had been layed off. A year into that effort I was effectively the only person in the company who understood how the library worked, and as such was the sole expert on it. Code reviews for checkins were still mandatory, but about the only thing any other reviewer would ever find were minor stylistic issues -- the real bugs went undetected because the expertise required to detect them just wasn't reasonable to expect of anyone else.

    Today I find myself working on deep close-to-the-metal code. Due to economic conditions it's just not feasible to have any overlap between engineers in the areas they're deeply familiar with (yes, that's a management issue -- city bus syndrome is a very real risk). For all but the smallest changes, others cannot fully understand the implications of my changes when they review my code, and I cannot understand the implications of the changes when I review their code. We still keep at it, but the effort rarely catches real problems.

    Alongside those issues, I have been with this company for more than ten years, yet in my particular group I'm the youngest person with the least experience. With such a mature and historied team, significant mistakes are exceedingly rare, and even minor mistakes are uncommon. The most common thing we catch is someone forgetting to update the current copyright date in a file's comment block.

    For the stated reasons, I currently view code reviews as generally just an extra hoop to jump through in the checkin process. If I should ever change jobs, I'm sure I'd once again appreciate having more experienced developers double-checking my efforts.

  • by Anonymous Brave Guy ( 457657 ) on Saturday June 13, 2009 @08:16PM (#28323559)

    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.

  • by jonwil ( 467024 ) on Saturday June 13, 2009 @08:25PM (#28323623)

    As a programmer, having someone else look at my code is good since there will always be mistakes I just cant see from looking at it. Someone else looking at it might pick them up when I cant, especially if they have more knowledge of bits of the codebase than I do.

  • by HomerJ ( 11142 ) on Saturday June 13, 2009 @08:32PM (#28323643)

    Which goes against the thinking for a lot of developers. They seem to take reviews of code personally, and believe everything they did is correct.

    I go the other way. If my code is good, it will stand the test of a review. If one or a group of my colleagues looks at my code and doesn't find a fault then I KNOW it's good. I don't have to just THINK it because I believe so. If I can't explain why I did something in a review, it shouldn't get into production code.

    Sometimes it's even simple stuff. I Do X, and someone goes "oh, we had to do it too, and wrote this bunch of code for it. Maybe we could combine the code into one usable module for both". It's stuff like that you can only really do in a good code review. It shouldn't JUST be done at a commit. It's something that should be part of the development process.

  • Re:yes! (Score:2, Insightful)

    by GeckoAddict ( 1154537 ) on Saturday June 13, 2009 @08:47PM (#28323719)
    Your last part I think is one of the most important. Making sure at least two Full-time employees (not contractors, not interns) know the code is extremely important in making sure that someone can answer questions should one leave/be out sick/etc. You also ensure that any assumptions you made about other parts of the system or other libraries are indeed correct, and you ensure code is using best practices for both performance and language standards. The key to code reviews is making sure it's a quick iterative diff of a few files, and not a room full of engineers looking at 50 files on an overhead projector. Our workplace recently instituted a new policy of 'every task/commit must have a code review' (it's actually a property of the task in our code management tool so it's easy to query), and it's helped us catch a lot of bugs before test ever sees it. A 5 minute fix from a code review is a lot better than finding it in test: spending 2 hours trying to dig through the logs to find the bad code, fixing it, and rebuilding everything so test can continue. Not to mention the code is a lot cleaner and a lot more optimized with a few people looking at it.
  • by MadShark ( 50912 ) on Saturday June 13, 2009 @08:52PM (#28323747)

    My company has a pretty rigid set of processes and we find that we catch approximately 25% of our defects in code reviews. This doesn't count minor things like not following the coding conventions or bad comments, those are logged separately. We have reviews where we hand them off to just one other engineer as well as the Fagan style of reviews, depending on what we feel is appropriate for the particular piece of code. In our industry(embedded software), it isn't exactly easy to push out a patch once the code is released so that plays into how/why we do it the way we do. I wouldn't say the review process is cheap, but neither are warranty campaigns. Pay it now or pay it later I guess...

    Even the best people occasionally typo, put in a bad/wrong comment or just flat out make a mistake during coding. Personally, I'd love it if we had the resources to review every single line of code I write.

  • by ciroknight ( 601098 ) on Saturday June 13, 2009 @10:10PM (#28324093)
    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 Unoti ( 731964 ) on Saturday June 13, 2009 @10:38PM (#28324175) Journal

    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.

  • by halcyon1234 ( 834388 ) <halcyon1234@hotmail.com> on Saturday June 13, 2009 @10:50PM (#28324225) Journal

    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.

  • by imasu ( 1008081 ) on Saturday June 13, 2009 @11:22PM (#28324339)
    The first case is canonical and often pointed to, but I find it problematic in many cases. Mentoring is always a good goal but I think that for real development work, if there is a large disparity between the skillsets it can lead to the more experienced engineer becoming frustrated and essentially feeling less productive (as the mentoree is not contributing much in the way of actual implementation). Similarly, it can be like drinking from a fire hose or the less experienced person. So while it is an effective technique in some cases, it is (in my experience) quite domain and problem specific, and if the gap in skills is too large, it simply does not work well. The second example you give is where it really can stand out though, in the spirit of the first example as well; bringing a second experienced dev up to speed in the first dev's domain.
  • by cetialphav ( 246516 ) on Saturday June 13, 2009 @11:49PM (#28324445)

    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.

  • by DMeans ( 756525 ) on Sunday June 14, 2009 @12:19AM (#28324577) Homepage
    I work for a very large software company, on a mission critical module used by very many companies. - How much do reviews cost? We don't care. - How many defects are found? Sometimes none. - How much does it cost when a bug exists? What you said. Code reviews are mission critical to my team. Software doesn't ship without them. Sometimes the code review finds the bug, sometimes it doesn't. But in the end, we have the check-box marked and it saves our bacon.
  • by wisty ( 1335733 ) on Sunday June 14, 2009 @12:24AM (#28324597)

    Pair programming is good for coding and software design (if you have a UI cases design), but crap for "UI, and model design by prototyping". If you want production code, you should use pair programming, design reviews, and code reviews. If you want to hack up a protoype (which you then refactor into a proper design, using pair programming, design reviews, and code reviews), then one person can be more creative than 2.

    Of course, some managers don't believe that development is a creative process, and think that they are the designers, but that's another problem.

  • by Anonymous Coward on Sunday June 14, 2009 @01:16AM (#28324799)

    +1

    A big part of the benefit comes from the coder putting in the extra effort to make the code review-ready. You may not get many changes coming out of the review itself, but the benefit has already been realized.

  • by zuperduperman ( 1206922 ) on Sunday June 14, 2009 @01:47AM (#28324873)

    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.

  • by man_of_mr_e ( 217855 ) on Sunday June 14, 2009 @04:27AM (#28325307)

    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.

  • Design != Coding (Score:2, Insightful)

    by Anonymous Coward on Sunday June 14, 2009 @05:12AM (#28325411)
    Until this is understood, additional discussion is futile.

    If pair programming is cost effective, you've hired the wrong programmers - pay more and get good developers (and, as a manager, set priorities to reward quality). Teach developers to desk check their own code -- every true "bug" found by your "pair" or missed by them but found by QA or the Customer should be viewed as a humiliating embarrassment by the coder - something that happens every few 10K of code but really requires a humble round of beer for the entire reveiw/QA team.

    Yes, part of Development is a creative process - but that's not an excuse for eliminating accountability or responsibility.
  • by Carewolf ( 581105 ) on Sunday June 14, 2009 @06:00AM (#28325533) Homepage

    Well, code reviews are boring to do, but has to be done be the best and most experienced developers usually the senior or lead developer. Some places doesn't understand that and just delegates this menial task to lower developers, or a team of inexperienced developers. Other places the top developer assigned the task, just skips through the code, because he thinks he has "better things to do".

  • Gods yes! (Score:3, Insightful)

    by Fastolfe ( 1470 ) on Sunday June 14, 2009 @11:57AM (#28326855)

    Just do them right:
    1. Each commit should have an explanation of what the change does, and should be small enough that the reviewer can do it quickly.
    2. Your organization should prioritize code reviews over other work; in many cases the review is blocking something.

    If your reviews are kept small, and are a high priority, they add enormous value and shouldn't negatively impact your work.

    Code reviews have the following perhaps non-obvious benefits:
    - They ensure the implementation does justice to the design
    - They help pass institutional knowledge to the developer ("This function has an existing implementation over here...")
    - They ensure code readability (especially when used with a formal style guide)
    - They help keep the developer honest, when he or she might take shortcuts or be lazy with a certain function.
    - By mandating code reviews, you have a pressure from the reviewers to keep each commit small, which encourages incremental development, which discovers design flaws early rather than after 10,000 lines are written.

    Code reviews aren't really a great place to FIND bugs. Yes, obvious bugs will stand out to an experienced developer, but the reviewer is another human, and he or she can easily miss the same bugs the developer missed. Really, unit tests are where you catch bugs, and a reviewer is usually in a better position to identify incomplete unit testing.

  • by Your.Master ( 1088569 ) on Sunday June 14, 2009 @07:17PM (#28330021)

    have no idea what company you work for but normally compaines don't hire inexperienced programmers.

    Where do you think experienced programmers come from?

    Yeah, yeah, cue the litany of Open Source projects. There's more than one kind of inexperienced, and Open Source doesn't prepare you for all aspects of business programming (nor vice-versa).

    Lots of companies have internships, hire kids out of school, hire experts in one domain to help bring their skills into another domain, etc..

    Also, near the end:

    We fire poor performers, we do not waste time and money on them.

    He said inexperienced programmers, not poor performers. There's an enormous difference. The concepts are almost orthogonal.

    I get a project specification on Jan 3rd and it needs to be in production in 13 weeks. I don't go out and hire inexperienced programmers for 13 weeks.

    So you do contract work only? Nobody is suggesting you do charity work on these cases. But it kind of answers your beginning question: "Why the hell would anyone in their right mind in business pursue #1"? Because they hire for a much longer term than 13 weeks. 13-week terms: that actually sounds like an educational stint.

The moon is made of green cheese. -- John Heywood

Working...