Naved Rehman
Posts: 1
Nickname: sapna2000
Registered: Dec, 2008
|
|
Re: Template for Code review guidelines
|
Posted: Dec 10, 2008 12:02 PM
|
|
Any updates...
> <br /><br />
> There was a recent blog about <a
> href="http://www.unix-girl.com/blog/archives/000862.html">c
> ode reviews</a>, which reminded me that I should post my
> code review guidelines to the net, for anyone to use. They
> are based on my own experience, and quite a lot of known
> research on the efficiency of code reviews. But in the end
> they reflect my personal ideas on how code reviews are
> best performed.
> <br /><br />
> I have had just limited success in introducing these
> styles of code reviews at the various companies I've
> worked at, which either reflects the sad state of the
> software industry, or that my ideas are just not
> implementable, or just bad luck. Whatever. Use the
> template below at your own risk. Replace "Xxxxxx" with the
> name of your company or organization, add your own ideas,
> and cut away stuff you don't like.
> <br /><br />
> <hr>
> <h1>Code Review Guidelines at Xxxxx</h1>
> This document is intended to start discussion about code
> review at Xxxxx.
> It primarily discusses code reviews, but design document
> reviews are, of
> course, even more important, since a design review can
> spot a problem even
> before coding starts. I don't have much experience in
> design reviews, so
> I'll stay out of that area in this document, but many of
> the points made
> below are valid also for design reviews.
> <h1>Introduction</h1>
> Code review is the process when a piece of code is
> scrutinized, primarily
> in the search for bugs, by one or more programmers not
> being the author
> (owner) of the code. Code reviews are, to my knowledge,
> the most effective
> way to find bugs (number of bugs found per hour of work).
> It has been reported to be about <b>3 times more effective
> than testing</b>, although it can be argued that the kind
> of bugs found during testing probably are more serious
> than the kind of bugs found during code reviews. It is
> also true that a bug found during a code review doesn't
> necessarily have to create a failure, since the buggy code
> may never be executed.
> <p>Unfortunately, code reviews easily ends up being time
> consuming and
> boring activities with unprepared people reviewing
> unprepared code. A few
> important guidelines are therefore necessary.
> </p><p>There are two different kinds of code reviews;
> <b>buddy review</b> and <b>formal review</b>. A buddy
> review is simply when the owner of a file passes it to a
> coworker for review. A formal review involves more people
> and is done in a meeting room at a specific time.
> </p><p>Code reviews must be <b>constructive</b>. Instead
> of saying "This code sucks" it should say "This code
> doesn't work because of X. I suggest rewriting it like
> this: Y". It takes a lot of energy to say "No" to a
> creative suggestion to rewrite a piece of code. Why say no
> to a better piece of code that can simply be pasted into
> the original file, replacing the old code?
> </p><p>The owner of the code has the last word if an issue
> cannot be resolved
> during code review. This is because the owner is more
> likely to know the
> full context of the problem solved in the code. This
> should also avoid
> the known problems with design-by-committee, i.e. when
> several people have
> to merge their different ideas for how a piece of code
> should be designed
> and implemented.
> </p><h1>Review objective</h1>
> The most important objective when reviewing code is to
> check for correctness, i.e. does the code do what it is
> supposed to do? The question is, of course, how can anyone
> know what a piece of code is supposed to do, unless there
> is some kind of documentation? Code implementation details
> are best documented in the code, as well structured
> JavaDoc comments. Design decisions and other lengthy
> discussions are best put into a separate file that in some
> way accompanies the code. They should be submitted for
> review at the same time as the code. Other worthwhile
> objectives during a code review are performance,
> portability, maintainability, testability, complexity,
> scalability, thread safety, etc. It is a good idea to
> explicitly mention such extra objectives, if they should
> be considered during a particular code review. Correctness
> is always the default. Any other objectives are extra.
> <h1>Code conventions</h1>
> There are many online coding standards for Java, and some
> of them can be
> worth reading before a code review, but we are using the
> Xxxxx Coding Standard.
> <p>Here is a list of other online coding standards for
> Java:
> </p><ul>
> <li>
> <a
> href="http://java.sun.com/docs/codeconv/html/CodeConvTOC.do
> c.html">Sun's
> Code Conventions for the JavaTM Programming
> Language</a></li>
>
> <li>
> <a
> href="http://www.infospheres.caltech.edu/resources/code_sta
> ndards/java_standard.html">The
> Infospheres Java Coding Standard</a></li>
>
> <li>
> <a
> href="http://developer.netscape.com/docs/technote/java/code
> style.html">Netscape's
> Software Coding Standards guide for Java</a></li>
>
> <li>
> <a
> href="http://www.ambysoft.com/javaCodingStandards.html">Amb
> ySoft Inc.
> Coding Standards for Java</a></li>
>
> <li>
> <a
> href="http://www.chimu.com/publications/javaStandards/index
> .html">ChiMu
> OO and Java Development: Guidelines and
> Resources</a></li>
>
> <li>
> <a
> href="http://www.macadamian.com/codingconventions.htm">Codi
> ng Conventions
> for C++ and Java applications (at Macadamian)</a></li>
>
> <li>
> <a
> href="http://gee.cs.oswego.edu/dl/html/javaCodingStd.html">
> Draft Java
> Coding Standard (by Doug Lea)</a></li>
>
> <li>
> <a href="http://geosoft.no/javastyle.html">Java
> Programming Style Guidelines
> (from Geosoft)</a></li>
>
> <li>
> <a href="http://www.multimania.com/beust/naming/">Naming
> Conventions in
> C++ and Jave : Can They Be Reconciled ?</a></li>
>
> <li>
> <a href="http://mindprod.com/unmain.html">How To Write
> Unmaintainable Code</a></li>
> </ul>
>
> <h1>Buddy review</h1>
> All code should be buddy reviewed. This can be done in
> many ways, but a
> very efficient way is to send email to the buddy, and
> copy/paste the code
> into the email. The buddy can then simply reply to the
> email and add comments in places where there is a review
> issue. The owner of the code can send the code to anyone
> he/she so chooses, but the buddy may not add any other
> addressees (doing this through BCC is strictly forbidden).
> This is to ensure that the owner of the code is not
> "punished" for initiating a code review by a buddy that
> passes unfavorable reviews to others. Code reviews are not
> a tool used in performance reviews! The name of the buddy
> should be
> added to the file after its review, preferably in a
> JavaDoc comment (<b><tt>@reviewed Mats
> Henricson</tt></b>).
> <p>Code passed to buddy review must <b>compile</b>. It
> must also be stable, meaning that is must not be changed
> while the buddy reviews the code, since it is very
> demoralizing to spend time improving code that no longer
> is current. This will force the owner of the code to
> reasonably clean up the code, which in itself is a good
> thing. It can be argued with great success that code
> should be reviewed as early as possible, maybe even before
> it is even close to ready, since a code review might
> reveal major design problems which of course are best
> fixed as early as possible. But it is also true that half
> finished code is very hard to review, since the overall
> design may only exist in the mind of the owner. Anyhow,
> it is up to the owner to decide when the code is ready for
> buddy review.
> </p><h1>Formal review</h1>
> Only key code needs to be formally reviewed. Key code is
> code used by many
> different parts of our system, is critical for the overall
> performance
> of the system, or is a vital interface between parts of
> the system.
> <p>Code should <b>never</b> be up for formal review unless
> it has passed a buddy review. This is to ensure that the
> code is in reasonable shape before it is sent to formal
> review. It is a huge waste of time to review code that is
> in poor shape, and it demoralizes the whole code review
> effort to attend such spectacles.
> </p><p>Petty issues, such as inconsistent style,
> misspelings and such, should
> not be discussed in formal reviews. Send them as an email
> instead and save
> everyone's time.
> </p><p>The meeting should have at least 3 and at most 5
> people, including the
> owner of the code. The optimal number of people at a
> formal code review
> has turned out to be 4 (if I remember correctly). If there
> are just 3 people attending, then only 2 of them have
> actually reviewed the code, since we can't count the
> owner. If there are more than 5 people attending, then the
> signal-to-noise ratio tends to be too low during the
> meeting.
> </p><p>The owner of the code is responsible for booking a
> room and specifying
> a time for the meeting. During the meeting, one person
> should be responsible for keeping discussions to the
> point, and cutting off threads that take too much time.
> Another person should be responsible for taking notes at
> the meeting. The owner of the code should <b>not</b> hold
> any of these positions since he/she will be fully occupied
> during the discussions.
> </p><p>Code passed to formal review must be stable, and
> should be submitted
> at least 2, and no more than 4 working days before the
> formal review meeting, which is long enough for a proper
> review, but short enough so that the code is still in
> working memory.</p>
|
|
Previous Topic
