Article 1606 of comp.software-eng: Path: intelisc!littlei!ogccse!blake!uw-beaver!cornell!batcomputer!rpi!leah!csd4.milw.wisc.edu!cs.utexas.edu!rutgers!bellcore!dduck!duncan From: duncan@dduck.ctt.bellcore.com (Scott Duncan) Newsgroups: comp.software-eng Subject: Re: code reviews Message-ID: <16925@bellcore.bellcore.com> Date: 19 Jun 89 14:38:24 GMT References: <12047@bloom-beacon.MIT.EDU> <116@opel.UUCP> Sender: news@bellcore.bellcore.com Reply-To: duncan@ctt.bellcore.com (Scott Duncan) Lines: 39 Posted: Mon Jun 19 07:38:24 1989 In article <116@opel.UUCP> johnk@opel.UUCP (John Kennedy) writes: > >I can't help but chuckle when I see code reviews and productivity in the >same paragraph. It's good for the project, it's what the customer wants, >but be assured that it's an increase in overhead and a decrease in productivity. I suppose that in terms of getting the initial release of the system out of implementation and into system testing (or out the door) that this is true. However, the contention of code reviews is that they assist in the quality (and reliability) of the code and that the code meets customer expectations more adequately. The experimental results and industry experience to which I have been exposed (at a number of companies, large and small) suggests that such reviews (in- cluding design reviews) tend to a more stable, less error-prone system over its lifecycle. Systems in the field also tend to cost many orders of magnitude more to fix than to have had corrected in implementation. People also point to the fact that such reviews are educational, provide more visibility to the programming effort, and engender a sense of responsibility for the system (not just an individual's piece of it) across the project. It also seems true that people who have "customers" think about and are con- cerned with quality issues while those who have "users" are more affected by productivity. Productivity, ultimately, is an internal concern while quality is an external one. As someone noted within the last week (and I paraphrase): in a year, they won't remember how fast you delivered it, just how good it is. And while there are definitely market considerations when it comes to releasing a product, I'd say they have to be accounted for by factoring such reviews (and other quality efforts) into the schedules. I would say that in the short-term, the claims of overhead and productivity "loss" could be substantiated. But in terms of satisfaction with the product, long-term cost, and productivity gains in less maintenance, I'd have to say such reviews end up being worth it. Speaking only for myself, of course, I am... Scott P. Duncan (duncan@ctt.bellcore.com OR ...!bellcore!ctt!duncan) (Bellcore, 444 Hoes Lane RRC 1H-210, Piscataway, NJ 08854) (201-699-3910 (w) 201-463-3683 (h)) Article 1607 of comp.software-eng: Path: intelisc!littlei!ogccse!husc6!rutgers!gatech!hubcap!ofut From: ofut@hubcap.clemson.edu (A. Jeff Offutt) Newsgroups: comp.software-eng Subject: Re: code reviews Message-ID: <5801@hubcap.clemson.edu> Date: 19 Jun 89 20:31:00 GMT References: <116@opel.UUCP> Organization: Department of Computer Science, Clemson University Lines: 41 Posted: Mon Jun 19 13:31:00 1989 From article <116@opel.UUCP>, by johnk@opel.UUCP (John Kennedy): > In article <12047@bloom-beacon.MIT.EDU> tada@athena.mit.edu (Michael Zehr) writes: > [...] >> >>Since part of my job here is to improve programmer productivity, i'm >>... >>periodic peer reviews. >> >>michael j zehr > I can't help but chuckle when I see code reviews and productivity in the > same paragraph. It's good for the project, it's what the customer wants, > but be assured that it's an increase in overhead and a decrease in productivity. Now be careful that when you say "increase in overhead and a decrease in productivity" we are all counting the same apples. It is true that code reviews will take time and energy away from other tasks, but it can also save time and energy later on. Reviewing code can significantly reduce the amount of time spent testing the software. In fact, some (inconclusive) studies have indicated that code reviews are a more cost-effective way of finding errors in software than traditional testing (other studies have shown the opposite). Whether or not you believe the studies completely, it is clear that reviewing code is a very effective way to find errors in the program. During a code review, the programmer can check that A) the software is somewhat related to the requirements and B) the requirements are reasonably close to what the customer wants and what the programmers can implement. This can significantly reduce the amount of time and money spent on maintaining the software. In short, saying that code reviews decrease productivity is only true over a very *small* part of the software lifecycle, when in fact, code reviews can decrease cost and increase quality over the *entire* lifecycle. (Of course, code reviews are also invariably tedious, boring and painful.) -- Jeff Offutt Department of CS, Clemson University, Clemson SC (803) 656-5882 Internet: ofut@hubcap.clemson.edu Article 1642 of comp.software-eng: Path: intelisc!omepd!mipos3!oliveb!apple!bionet!csd4.milw.wisc.edu!lll-winken!uunet!mcvax!ukc!stl!idec!marlow!cweir From: cweir@marlow.uucp (Charles Weir) Newsgroups: comp.software-eng Subject: Code reviews: a suggested approach. Summary: Describes 1 to 1 approach for code reviews Keywords: review coding code qa quality-assurance Message-ID: <651@alice.marlow.uucp> Date: 26 Jun 89 12:30:34 GMT References: <12047@bloom-beacon.MIT.EDU> Reply-To: cweir%marlow.UUCP@idec.stc.co.uk (Charles Weir) Organization: Reuters Ltd PLC, Marlow, Bucks, England Lines: 102 Suggested approach for a Code Walkthrough Following the discussion about code reviews, here is a document I put together a little while back. I should be interested to find out how it compares with other people's experiences. We had been using code walkthroughs to find bugs in code where testing was proving unsuccessful. Sometimes they were very successful, sometimes not. This note attempts do analyse what leads to a successful review. Our targets for a walkthrough are:- * No Bugs. * No indeterminate situations. * Maintainable and understandable code (as for as convenient) . A bug is a straightforward point where under some situation the code will not work as the programmer intended. An "indeterminate situation" is where the code behaviour is unpredictable (technically this is a bug too). How we go about it ------------------ We used the following guidelines:- A code walkthrough is constructive. The walker needs to avoid attacking or threatening the programmer either directly or indirectly. The walker must focus attention on a particular problem, a particular line of code or element. We avoid issues of approach and style ("I wouldn't do it like this..."), or unnecessary performance enhancement ("It'll be faster if..."). We tackle problems using questions rather than statements:- * How Is this function defined? * How does this work? * What parameters are being passed here? * When is this called and by what? * I think I may see a problem here. Am I right? (N.b. Quite often the answer will be no) The process of questioning the code is more effective than a straight challenge. It is non-threatening. And it encourages the programmer to look at his/her code from a different perspective. The inspections should not last too long. The mental effort required is similar to University entrance examinations, so over-stretching here oneself does nothing for anyone. A sensible maximum is two hours for a session or less. It is much better to inspect a body of code bit by bit in a number of short inspections over a week or two, than all at once. Further Suggestions (particularly for C code reviews) ----------------------------------------------------- Bug hunting too is partly a matter of habitat. It is important to look at the places where bugs live. We look mostly at:- * Buffer, string, and arrays. Could there be overruns? Zero termination of strings? "strlen" etc. * Error Handling: Is an error ignored? Does the handling mechanism work? * Statuses: Are there states (combinations of statuses and flags etc.) which can occur but are not handled? Or which are handled but could never occur? * Double use: Same data structure used by conflicting processes. Flags and status variables used for two purposes. * Comments: Misleading or out-of-date? It is often worth covering up comments when inspecting the code. For the inspection to be effective, it is not necessary for the reviewer fully to understand the code and the design. The questioning process uses the programmer's knowledge to supply that information. It is important to keep a sense of perspective. In C, for example, it is important to replace magic numbers with "#defines" for maintenance purposes: This is not the same as a bug. Function headers and comments generally are necessary for future maintenance: Again these are not bugs. And we shouldn't treat them as such. The GOTCHA trap is a classic: It's easy to communicate the sense of victory at finding a difficult bug in a way which threatens the programmer ("GOTCHA"). Again we use questions to communicate, rather than statements. A sense of humour is important too. A laugh both defuses the tension, and makes the process more fun. -- Charles Weir, Reuters Limited, 85 Fleet Street, London EC4P 4AJ Tel: +44+1+324 6231 cweir@marlow.uucp OR cweir%uucp.marlow@idec.stc.co.uk {backbone}!mcvax!marlow.uucp!cweir Article 1670 of comp.software-eng: Path: intelisc!littlei!ogccse!blake!uw-beaver!cornell!rochester!pt.cs.cmu.edu!sei!sei.cmu.edu!mcp From: mcp@sei.cmu.edu (Mark Paulk) Newsgroups: comp.software-eng Subject: Re: code reviews Summary: references and data on reviews/inspections Keywords: peer review, review, inspection, walkthrough Message-ID: <9474@aw.sei.cmu.edu> Date: 20 Jun 89 16:42:41 GMT References: <116@opel.UUCP> <5801@hubcap.clemson.edu> Sender: netnews@sei.cmu.edu Reply-To: mcp@bs.sei.cmu.edu.UUCP (PUT YOUR NAME HERE) Organization: Carnegie-Mellon University, SEI, Pgh, Pa Lines: 94 Posted: Tue Jun 20 09:42:41 1989 In article <5801@hubcap.clemson.edu> ofut@hubcap.clemson.edu (A. Jeff Offutt) writes: >From article <116@opel.UUCP>, by johnk@opel.UUCP (John Kennedy): >> In article <12047@bloom-beacon.MIT.EDU> tada@athena.mit.edu (Michael Zehr) writes: >> [...] >>> >>>Since part of my job here is to improve programmer productivity, i'm >>>... >>>periodic peer reviews. >>> >>>michael j zehr > > >> I can't help but chuckle when I see code reviews and productivity in the >> same paragraph. It's good for the project, it's what the customer wants, > >Now be careful that when you say "increase in overhead and a decrease in >productivity" we are all counting the same apples. It is true that >code reviews will take time and energy away from other tasks, but it can >also save time and energy later on. Reviewing code can significantly reduce The following references have some interesting data and insights on the role and power of inspections. Unfortunately it is difficult to get hard data on the effectiveness of methods such as inspections, but there has been some work published which is much more pertinent than my opinion would be... Pertinent quotes follow each reference. Let me also emphasize correct definition of terms! Inspections, reviews, walkthroughs, etc., are frequently overloaded terms. For the "official" definitions, see IEEE Standard 1028 (or Fagan's original paper for that matter). There are distinct differences between the concepts; the most formal is inspection. I use peer review as the all inclusive term because it is not defined in 1028, but some people use it in the hierarchy desk check peer review walkthrough inspection as meaning a 1-1 review with A peer (as opposed to a group of peers). Review, by itself, covers a wide range of sins. Much of this discussion has been oriented at technical reviews, i.e., CDR, PDR class reviews, which are NOT what the original poster was asking about. @b([ACKE89]) A.F. Ackerman, L.S. Buchwald, and F.H. Lewski, "Software Inspections: An Effective Verification Process," IEEE Software, Vol. 6, No. 3, May, 1989, pp. 31-36. Software inspections have been found to be superior to reviews and walkthroughs, p. 31 collection and analysis of data for immediate and long-term process improvement, p. 32 inspections improve quality and productivity, p. 34 inspections give project management more definite and more dependable milestones than less formal review processes, p. 35 @b([GILB88]) T. Gilb, PRINCIPLES OF SOFTWARE ENGINEERING MANAGEMENT, Addison-Wesley, Reading, MA, 1988. The inspection method is the most effective quality control method for software specification documentation we know about. p. 68 Testing is a maximum of 50 to 55% effective at defect identification and removal for a single test process. p. 221 Inspection is about 80% (+/- 20%) effective in removing existing defects. p. 221 The average is five hours saved for every hour invested in inspection. p. 221 Inspected programs were ten times cheaper to maintain than ... similar non-inspected programs. p. 221 High level inspections (requirements and design specification) were the most powerful things they could do. p. 244 @b([GRAD87]) R.B. Grady and D.L. Caswell, SOFTWARE METRICS: ESTABLISHING A COMPANY-WIDE PROGRAM, Prentice-Hall, Englewood Cliffs, NJ, 1987. The five "modern programming practices" which had the strongest correlation with productivity were top-down design, modular design, design reviews, code inspections, and quality assurance programs. p. 20 Projects of highest productivity are among those with the lowest defect densities. p. 140 @b([MYER88]) W. Myers, "Shuttle code achieves very low error rate," IEEE Software, Vol. 5, No. 5, September, 1988, pp. 93-95. 500K Space Shuttle source code with 0.11 errors/KSLOC credited by IBM/FSD to process definition, rigorous inspection of work products across the process, independent software verification, defect-cause analysis, knowledge engineering, expert systems, specialized tools, and "good old value gained from lessons learned." (Barbara Kolkhorst) Reduced from 2 errors/KSLOC, versus 8-10 errors/KSLOC for industry. Effort/time spent on software reconfiguration reduced by 50%. About 85% of major errors discovered in inspection. -- Mark C. Paulk mcp@sei.cmu.edu "The essence of true adulthood is deferred gratification." Article 1671 of comp.software-eng: Path: intelisc!littlei!ogccse!blake!uw-beaver!cornell!mailrus!uflorida!gatech!ncsuvx!mcnc!duke!romeo!crm From: crm@romeo.cs.duke.edu (Charlie Martin) Newsgroups: comp.software-eng Subject: Re: code reviews Message-ID: <14810@duke.cs.duke.edu> Date: 20 Jun 89 18:39:33 GMT References: <12047@bloom-beacon.MIT.EDU> <116@opel.UUCP> Sender: news@duke.cs.duke.edu Reply-To: crm@romeo.UUCP (Charlie Martin) Organization: Duke University CS Dept.; Durham, NC Lines: 36 Posted: Tue Jun 20 11:39:33 1989 In article <116@opel.UUCP> johnk@opel.UUCP (John Kennedy) writes: >In article <12047@bloom-beacon.MIT.EDU> tada@athena.mit.edu (Michael Zehr) writes: >[...] >> >>Since part of my job here is to improve programmer productivity, i'm >>interested in eventually implementing some sort of procedure for >>periodic peer reviews. Does anyone have suggestions or >>reccommendations? >> >>michael j zehr > > >I can't help but chuckle when I see code reviews and productivity in the >same paragraph. It's good for the project, it's what the customer wants, >but be assured that it's an increase in overhead and a decrease in productivity. > >I wish this weren't true. > >John The way I've heard it is that the use of formal reviews has the effect of lowering lifecycle cost and cost considered through delivery. It does have the effect of increasing overhead cost *during* design and coding, but most methodologies have that effect. The point here is that coding cost is only about 15% of total cost in waterfall methodologies (harder to evaluate in, say, incremental development) and it's worth while to increase coding cost by 25% (so overall increase is 5%) to gain 10-20 % over all. As usual, I'm not typing this where I can lay hands on the references, but several studies have been done that show of all methods studied that can be applied to software development, reviews have the greatest impact on cost reduction. Charlie Martin (crm@cs.duke.edu,mcnc!duke!crm) Article 1672 of comp.software-eng: Path: intelisc!littlei!ogccse!blake!uw-beaver!apollo!perry From: perry@apollo.COM (Jim Perry) Newsgroups: comp.software-eng Subject: Re: code reviews Message-ID: <43f2d7c0.183dc@apollo.COM> Date: 20 Jun 89 19:26:00 GMT References: <12047@bloom-beacon.MIT.EDU> <116@opel.UUCP> <13716@umn-cs.CS.UMN.EDU> Reply-To: perry@apollo.COM (Jim Perry) Organization: Apollo Division of Hewlett-Packard, Chelmsford, MA Lines: 64 Posted: Tue Jun 20 12:26:00 1989 In article <13716@umn-cs.CS.UMN.EDU> meuer@klein.UUCP (Mark Meuer) writes: >In article <116@opel.UUCP> johnk@opel.UUCP (John Kennedy) writes: >> >>I can't help but chuckle when I see code reviews and productivity in the >>same paragraph. It's good for the project, it's what the customer wants, >>but be assured that it's an increase in overhead and a decrease in productivity. >> >>I wish this weren't true. >> >>John > >I beg to differ. I used to work for a software company where peer >reviews were used extensively. They were very helpful in a number of >respects, and were well worth the effort. > >It is true that reviews add overhead to the development, but that time >is made up for by better designs and much better communication and >understanding between members of the programming team. > >-mark I'll second that. Many people seem to think of reviews as either a very formal process where you're going through a lot of unnecessary formality to satisfy some external requirement, or as an antagonistic process with a manager poring over your code like a hostile grader. Clearly neither of these situations are likely to yield a good software engineering environment, which is not to say that there aren't sites out there run that way. My experience of peer reviews is just that: review of code (or a design or whatever) by one or more peers -- engineers whose ability and insights you respect similarly to your own. Just as with writing English, one becomes blind to certain aspects of one's own writing over time, and having a fresh eye look over it can turn up flaws, inconsistencies, etc. It's not an antagonistic process, and you don't expect to turn up lots of problems, but you'd be surprised at the number of things you *can* turn up this way, even in the best code. A very real additional benefit of such an environment: where code is written with the expectation that others will be reading it, the tendency is toward code that is more readable, more understandable, and better documented than otherwise, since it must be understood by a reader who is generally not as familiar with the code as the writer. Since each writer is also a reader, this is to everyone's benefit. Of course all of those aspects are in direct support of future maintainance of such code at very little incremental cost. While I think there's more room for formality in reviewing of design specs and suchlike, code reviews can be quite informal, for instance just having a colleague look over the source/listing of a module, and diffs from the previous release where appropriate, as part of the release/freeze process. It helps if there's a librarian/release coordinator to coordinate the process of generating listings/diffs and allocating reviewers, but even an informal process ("hey, Joe, want to take a look at this?") is better than never having anyone look at your code. It generally doesn't take more than a few minutes to review most coding updates, and even reading a completely new module at this level should take under an hour. (Remember, you trust the writer, and assume it works, you're just double-checking, not grading.) That's pretty minimal overhead, for a pretty big payback. -- Jim Perry perry@apollo.com HP/Apollo, Chelmsford MA This particularly rapid unintelligible patter isn't generally heard and if it is it doesn't matter.