Forgot your password?
typodupeerror

Are Code Reviews Worth It? 345

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

Are Code Reviews Worth It?

Comments Filter:
  • by Anrego (830717) * on Saturday June 13, 2009 @05:26PM (#28322533)

    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

    • by Tx (96709) on Saturday June 13, 2009 @05:31PM (#28322567) Journal

      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?

      • Re: (Score:3, Insightful)

        by Anrego (830717) *

        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.

      • You are not funny with those reviews of reviews! *sigh* Now I will have to metamoderate your Funny moderation down.
      • Re: (Score:3, Funny)

        by MrMista_B (891430)

        Well, in that case, we should do a thorougly review of the reviews of the reviewed review reviews.

        BUFFER OVERRIDE

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

    • Re: (Score:3, Informative)

      by Skuld-Chan (302449)

      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: (Score:3, Insightful)

        by Carewolf (581105)

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

    • by piojo (995934) on Saturday June 13, 2009 @05:57PM (#28322779)

      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.

      • 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 Braino420 (896819)

          Its also a great way to mentor newer developers and teach them the tricks of the trade.

          Agreed. I'm a junior dev, and I have learned a few things from our group's team lead during code reviews. It also helps in the communication between the devs and the team lead. For instance, if some library code we're using needs refactoring because it's making our code awkward, it's helps him identify if we should possibly spend some time working on that to help the whole team write better code.

      • by DrLang21 (900992)
        I think this needs to be done with great care. Suggestions can always be made, but if you halt progress to fix some code that works reliably for the sake of cleanliness, you can quickly get bogged down in word-smithing that adds little value. Findings in code reviews need to be prioritized. If it works but is written a bit ugly, make a note of it. If it works but has unacceptable holes in it, put it on a moderate priority. If it does not do what is intended at all, put it at the top of the list. And so
    • by grahamsz (150076) on Saturday June 13, 2009 @06:58PM (#28323119) Homepage Journal

      Damn europeans and their metric system.

      How many shitloads are in a fucktonne?

    • Re: (Score:3, Insightful)

      by cetialphav (246516)

      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 trac

  • Yes! (Score:5, Interesting)

    by Omnifarious (11933) * <eric-slash AT omnifarious DOT org> on Saturday June 13, 2009 @05:29PM (#28322547) Homepage Journal

    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.

    • Re: (Score:2, Interesting)

      by Gorgeous Si (594753)
      Where I work, we have code reviews before checking in non-trivial changes. We don't have design reviews though, which I think are more worthwhile. If a problem is found pre-checkin, then it's usually too late to go back and re-code the work - meaning that these issues are recorded by the coder and should be looked at again when possible; however, with the constant deadlines we work to, there often isn't time to go back and make the improvements. I think design reviews are worthwhile for verifying the work
    • Re: (Score:3, Interesting)

      by ers81239 (94163)

      I just want to highlight your second point. I believe that THE most important thing gained from code reviews is the spreading knowledge and gaining understanding. New development is always great, but most programming is maintaining/fixing/improving existing projects. A code review is a great way to really learn about code readability. You actually get to see other people read your code and you get to read other people's code. All of this code is fresh in someone's mind so it can be explained, and how t

  • 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.
  • When done right (Score:5, Informative)

    by El_Muerte_TDS (592157) <elmuerte@@@drunksnipers...com> on Saturday June 13, 2009 @05:36PM (#28322601) Homepage

    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.

  • 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.
    • Re: (Score:2, Funny)

      by pankkake (877909)
      Grammar Nazi to the rescue: If your code review
    • My code is self reviewing.

    • Re: (Score:2, Insightful)

      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 pr

  • by Anonymous Coward on Saturday June 13, 2009 @05: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...

  • 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 Qubit (100461)

      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.

      This is basically how we work at my lab. Anything going into the master branch of our multimedia engine needs peer review; anything going into a multimedia project needs peer review after we've hit beta (roughly speaking).

      In my experience code review is a very helpful part of development. It often catches stupid errors and can be an important piece of keeping the programming team abreast of everything that's going into the codebase.

      If you don't like unit tests (or at least the idea of writing unit tests --

  • by Anonymous Coward on Saturday June 13, 2009 @05: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..

  • My group started implementing code reviews this month. To get in the swing of things, we decided to do a test review of existing foundation code, stuff that had been in the product for many months, had gone through multiple QA cycles, and had been shipped to the customer as part of the first "production" release.

    In the first hour-long review, we found a number of significant issues, and one full-blown bug.

  • by ipoverscsi (523760) on Saturday June 13, 2009 @05:40PM (#28322637)

    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.

  • by QuoteMstr (55051) <dan.colascione@gmail.com> on Saturday June 13, 2009 @05:40PM (#28322643)

    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)

      by readin (838620)

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

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

    • by hedwards (940851)
      Not to mention the incentive that regular reviews provide for concise and well documented code. I can't imagine some of the examples I've seen of comments run amok or code only understandable at the Balmer peek.

      Code reviews aren't going to solve that by themselves, but if you're looking at the code as part of a program of review on a reasonable basis, it's harder for bits to fester in odd corners of the repository.
    • Re: (Score:3, Insightful)

      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 curre

  • 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.
  • PCI-DSS Code Reviews (Score:3, Interesting)

    by Anonymous Coward on Saturday June 13, 2009 @05: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.

  • then code reviews would be redundant. Sure, testing first is expensive. But probably no more expensive than code reviews. If you have a QA person or team checking the final product, then your bases are covered.
    • Re: (Score:3, Insightful)

      by hedwards (940851)
      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.
      • Re: (Score:3, Interesting)

        Naming conventions are overrated. Most programmers outside Microsoft have thrown Hungarian notation out the window, and other forms as well. If your code is well-written in the first place, most naming conventions are simply verbose redundancy. It's an old-school habit that is largely no longer necessary or followed. (Of course, that does depend on what kind of coding you are doing. If you are still doing C++ in Visual Studio 3 you might want to stick with it.)

        Second, modern tests are documentation. For
    • by Nerdfest (867930)
      It is not redundant. You can use TDD and still have developers that write unmaintainable crap.
      • Then you have coders who belong elsewhere. Why put up with that?

        ANY kind of programming can be done sloppily or poorly. That is not an argument for or against any particular methodology.
    • by radtea (464814) on Saturday June 13, 2009 @06:10PM (#28322849)

      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.

      • Okay, I admit that I may be biased, but I disagree. If you have "commenting standards" at all, for example, that is an indication you are not using modern programming methods.

        No, test-driven-development is not perfect. But it's a hell of a lot better than pretty much anything else that has so far been devised. And code reviews do not tend to catch many coding mistakes that TDD misses... but QA can.

        "A while back on /. we had a story about a serious bug in a major product (can't remember what it was) an
  • How many per cent of your programmers change, i.e. move to other companies, every year? The higher rate, the more need for well written, easy-to-read code.

  • I have no doubts at all that code reviews are highly useful. They can discover coding errors that are insanely hard to discover other ways. In anything that was critical, I would advocate them without the slightest hesitation. However, since this about non-critical stuff, then it is far more questionable. I think you should still use them, but on more limit basis. There will always be the tasks that are highly technical internally that need a good thorough looking through the code as well as UI test, b
  • 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).

  • Depends on the team, project and needs.

    If you have padawan coders, or coders from another corporation.. code reviews matter.. stylistically they wont mesh, they may have some foreign habits that you need to either squash or embrace. These may be the people who do a C++ style for loop in perl when it is not appropriate. These may be best-practices nazis. These all warrant code review.

    If you are modifying byzantine code from the people who write HP office-jet software.. you need code review.

    If youre

  • 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.
  • I've led large teams of developers and I guarantee that even if you write coding standards, or have a common coding style for the project that somebody in your team will either ignore it, or do their own thing. The guidelines might say don't use hungarian but somebody still will. Code reviews are an excellent way to enforce standards and ensure a base level of code quality across the board. Aside from general style, reviews mean you catch people not programming defensively, not catching exceptions, not writ
  • Code reviews are worth it but often the "cost" of doing proper reviews are underestimated. To implement results of the review may cost a bit of time and money if not another review where if project managers and planners just assume its a "fast fix" then problems will arrise.

  • For a project that's more than one or two people, code reviews are vital. They enforce consistency in style and methodology. They also spread knowledge. They mitigate the "bus factor" (how negatively impacted your project will be if someone is hit by a bus). I've worked professionally at two companies. One of them did not have code reviews. Every time someone left the company, the project either grinds to a halt or they have to rewrite that person's work because no one else can understand it. There w
  • 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.

  • "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 CuteSteveJobs (1343851) on Saturday June 13, 2009 @06:21PM (#28322917)
    When I was working as a trainee, I was invited to the code review of a mediocre programmer. A bunch of people were there including a senior A/P who was my boss. My boss started ripping into it and I quickly realized the mediocre programmer - who couldn't code his way out of a wet paper bag - had taken one of my programs and tried changing it (including my name). It didn't work and the senior A/P was asking him 'WTF does this do?'

    Oh the dilemma. Do I take ownership and defend "my" code, or say nothing and enjoy watching him squirm? I did the latter watching him go "ummmmm ummmmm ummmmm". Finally medicore programmer admitted he copied it and it was really mine. I said "Well it was working when you copied it off me."

    Needless to say he ended up being promoted to manager. Not even a PHB which requires a bit of cunning, he was just a flat out idiot. I keep in touch with some of my coworkers to this day and whenever we want to describe a certain type of idiot we use his name. Duuuuuuuuuuuuuuh.
    • by timmarhy (659436)
      it's hard to believe anyone doesn't think code reviews are worthwhile. without them all it takes is one retard to destroy a company.
  • In my experience, code reviews are generally useless. Main problem is that very few software projects have decent documentation which can be used as base for code review. Without documentation, reviews generally end up being a R&D-wide (or worse) squabble.

    As development manager, one should try to make the reviews an integral part of development process. Keyword is "integral." Results of review are hard to quantify, thus something should be done to make them useful to the other phases of development

  • In my shop, there is no formal code review process. But informally, we all have to maintain this stuff, and there aren't enough of us that everyone gets one nice, clearly demarcated area of responsibility. So in that principle, there is a strong informal process.

    In the course of designing a piece of software, or subsystem there is a lot of "Okay, here is my approach. See any pitfalls?" During implementation, there is a lot of over the shoulder "could you take this over?". And once a piece of code is ready t

  • WTF does maybe mean? (Score:5, Interesting)

    by lalena (1221394) on Saturday June 13, 2009 @07:25PM (#28323261) Homepage
    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.
  • 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.

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

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

HOST SYSTEM NOT RESPONDING, PROBABLY DOWN. DO YOU WANT TO WAIT? (Y/N)

Working...