This week's book giveaway is in the Servlets forum.
We're giving away four copies of Murach's Java Servlets and JSP and have Joel Murach on-line!
See this thread for details.
The moose likes Agile and Other Processes and the fly likes Code Review process Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Murach's Java Servlets and JSP this week in the Servlets forum!
JavaRanch » Java Forums » Engineering » Agile and Other Processes
Bookmark "Code Review process" Watch "Code Review process" New topic
Author

Code Review process

Kenny Birney
Greenhorn

Joined: Apr 27, 2004
Posts: 9
Hi all,

Ive been tasked with writing a formal code review policy, as in the steps that will be followed during the review.

What sort of things would you check during a code review? What procedures do you follow?

Thanks for any help

Kenny Birney
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
First identify your objectives. These are not all well served by the same review: inspection for defects, checking compliance to standards, knowledge transfer, refactoring advice, etc.

Steve McConnell's Code Complete has a good overview of several different review types with the goals, roles, preparation and expectations and such.

What have you found with Google?


A good question is never answered. It is not a bolt to be tightened into place but a seed to be planted and to bear more seed toward the hope of greening the landscape of the idea. John Ciardi
Kenny Birney
Greenhorn

Joined: Apr 27, 2004
Posts: 9
Well as usual with XP practices, ive found loads of stuff telling me how good the practice is. but very, very little on how to actually perform it.
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
By coincidence, Ineeded to read up on this subject for my project, too. Here are a few things from Google that I plan to read later today ...

http://www.possibility.com/epowiki/Wiki.jsp?page=CodeReviews

http://www.possibility.com/epowiki/Wiki.jsp?page=CodeReviewReferences

http://people.msoe.edu/~barnicks/courses/cs489/codereview.htm

http://www.mozilla.org/hacking/code-review-faq.html

http://www.intelligententerprise.com/print_article_flat.jhtml?article=/031210/619e_business1_1.jhtml

http://craig.afox.org/index.php/F05SENG411_Code_Reviews

http://meet-joe-bloggs.blogspot.com/2006/02/episode-6-code-review.html

http://www.stellman-greene.com/aspm/content/blogcategory/32/40/

http://wiki.openlaszlo.org/Code_Review_Process
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Originally posted by Kenny Birney:
Well as usual with XP practices, ive found loads of stuff telling me how good the practice is. but very, very little on how to actually perform it.


I'm confused. As far as I know, formal code reviews aren't an XP practice.


The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
Scott Ambler
author
Ranch Hand

Joined: Dec 12, 2003
Posts: 608
Formal code reviews aren't an XP practice, probably because like artifact reviews in general they are likely a process smell.

- Scott


<a href="http://www-306.ibm.com/software/rational/bios/ambler.html" target="_blank" rel="nofollow">Scott W. Ambler</a><br />Practice Leader Agile Development, IBM Rational<br /> <br />Now available: <a href="http://www.ambysoft.com/books/refactoringDatabases.html" target="_blank" rel="nofollow">Refactoring Databases: Evolutionary Database Design</a>
Stan James
(instanceof Sidekick)
Ranch Hand

Joined: Jan 29, 2003
Posts: 8791
I suppose pairing generally eliminates the need for reviews, or does continuous reviews or however you want to put it. If you're not pairing, reviews are one alternative way to improve quality.

Some of those links point to studies that asynchronous reviews (reviewers reading at their own desk, at their own leisure) are more effective than group reviews. Idunno about that.

BTW: If you skip all the other links in my list, visit Joe Bloggs.
Shameer Subedar
Greenhorn

Joined: Apr 08, 2004
Posts: 22
You may Mr Fagan article useful.



Hope this helps,
Shameer
Lasse Koskela
author
Sheriff

Joined: Jan 23, 2002
Posts: 11962
    
    5
Originally posted by Shameer Subedar:
You may Mr Fagan article useful.

a paper on Fagan inspections

Shameer, have you used Fagan inspections yourself? If you have, how do you feel about them? Have you tried other, more informal code review techniques? If you have, how would you compare the effort and results between them?


Author of Test Driven (2007) and Effective Unit Testing (2013) [Blog] [HowToAskQuestionsOnJavaRanch]
Shameer Subedar
Greenhorn

Joined: Apr 08, 2004
Posts: 22
Lasse, when I was working with Motorola they had a detailed Software Production Process and code reviews was one of the processes. Initially we had formal code reviews and informal code reviews. This was changed before we went for a CMM assessment and we only had formal code reviews.

Basically, informal code reviews were ad-hoc in that Tech lead or someone senior would review the code of a junior engineer. I had no issues with this and thought it was good as Tech lead pointed things out and was not intermediating as a formal review with 3 or more people.

The Formal reviews were based on the Fagan style, which included about 7 phases. The reviews were called inspections as this process was applied to any software artefact that was produced.
1. Planning - set a time in the schedule with PM
2. Inspection kit - author checks code, code is labelled etc and sends details to reviewers
3. Overview - given by author to reviewers
4. Preparation - individuals review on there own
5. Inspection - all meet and discuss the artefact
6. Rework/moderator check
7. Process improvement (optional if we went over/under the control limits)

In addition, the process was modified that defects were captured and analysed with ODC (Orthogonal Defect Classification)

In the main, I found this process very useful in detecting defects. The main benefits were IMO:
- Bringing new developers up to seed on company coding standards and introducing them to the business rules.
- More than one person inspecting the code, many minds are always better than one and you get a good result when everyone is focussed and have prepared for the inspection.
- Collection of metrics, very useful in seeing trends and common errors that are made.

Major disadvantages are:
- Control limits if you go over and under them, management then ask questions. For example you inspected 250 LOC of code in half an hour, not the prescribed 2 hours. A justification may then be required.
- Can be a very long process if there small change in code < 20 LOC

Having moved on from the tech sector to the finance sector, code reviews are very ad-hoc and it all depends on the team leader and type they want. In general it just one person who looks at the code, mostly the team lead. They spend at max about 10-20 minutes. These types of reviews don't provide much benefit because the team lead does not always know specific business rule, hence if there are errors it doesn't get picked up and just gets raised as a defect.

A practise that I employee is get a fellow team member that is familiar with code/business rules and get him to review the code at my desk. I generally walk through the code with him and let them ask question why this done etc. I have found this very effective as this does pick defects or incorrect business logic.

In summary, getting people together that are familiar with the code, business rules and with the correct attitude will make a code review/inspection very beneficial in improving the quality of the code..
Lasse Koskela
author
Sheriff

Joined: Jan 23, 2002
Posts: 11962
    
    5
So then the question remains, were the Fagan inspections more bang-for-buck than what you're doing with taking a coworker to your desk, considering the amount of process and time involved?
Shameer Subedar
Greenhorn

Joined: Apr 08, 2004
Posts: 22
Due to my constraints I would like more people to peer review my work. So yes, the co-worker option as an immediate affect and helps meet the deadline(s).

However, if the process was formal, metrics could be collected, knowledge spread to other team members, technical skill learned from reading other peoples code etc. I believe this has more benefits to the team/company.

IMO, I would agree and prefer a Fagan style inspection, if customized to your needs it would provide more benefits in the long run.

This is all based on my experience and would like to know what other people employ in regards to code reviews. The most important factor in both processes is that that the correct people are involved and have the correct attitude towards code reviews/inspections. People do play a big part in both processes

[ May 16, 2006: Message edited by: Shameer Subedar ]
[ May 16, 2006: Message edited by: Shameer Subedar ]
Ilja Preuss
author
Sheriff

Joined: Jul 11, 2001
Posts: 14112
Originally posted by Shameer Subedar:

This is all based on my experience and would like to know what other people employ in regards to code reviews.


We are doing a lot of Pair Programming. We also do informal peer reviews from time to time. I think that Pair Programming generally works better. I suspect that it would work even better if we switched pairs more frequently (but there is some resistance to this idea in our team).

We never tried formal reviews.
Lasse Koskela
author
Sheriff

Joined: Jan 23, 2002
Posts: 11962
    
    5
I don't have any prolonged experience with formal reviews (e.g. Fagan inspections) either. On one project many, many years ago there was an attempt at adopting more formal reviews but it was quickly abandoned as people felt the reviews weren't helping. After that, just like Ilja, I've been having positive experiences with more informal methods of peer review. Pair programming is number one for me but we've also had a good feeling about "micro" reviews a la someone reviews each and every SVN commit made during the past couple of hours.
Fintan Conway
Ranch Hand

Joined: Apr 03, 2002
Posts: 141
Hi Kenny,

One way to approach this formally is to assign different reviewers a different aspect of the code to review. They should be given the code to review 24-48 hours before the review meeting. At the meeting each reviewer highlights the issues that they have found.

The meeting is used to highlight any issues, not to have a prolonged discussion on what the developer needs to do to resolve the issue. This should be done outside of the review meeting. This combined with the fact the the reviews have been completed before the meeting means that the review meeting is kept relatively short.

Some aspects that can be reviewed :
Conformance to Code Guidelines
Peformance issues in the code
Code keeps to project specific guidelines
Inefficient algorithms (A senior developer can show better methods of acheiving the same thing)
Localisation issues
User Interface conforms to UI guidelines
Functionality conforms to requirements

HTH,

Fintan
 
wood burning stoves
 
subject: Code Review process
 
Similar Threads
Cyclic dependency
Help Needed in EL
what is the meaning of code review in java??
Code Review
What is the best way to review the questions in final exam