• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Code Review process

 
Greenhorn
Posts: 9
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
 
(instanceof Sidekick)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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?
 
Kenny Birney
Greenhorn
Posts: 9
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
 
author
Posts: 14112
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.
 
author
Posts: 608
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Formal code reviews aren't an XP practice, probably because like artifact reviews in general they are likely a process smell.

- Scott
 
Stan James
(instanceof Sidekick)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
 
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You may Mr Fagan article useful.



Hope this helps,
Shameer
 
author
Posts: 11962
5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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?
 
Shameer Subedar
Greenhorn
Posts: 22
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Posts: 11962
5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Posts: 22
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Posts: 14112
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Posts: 11962
5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
 
Ranch Hand
Posts: 150
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
 
Consider Paul's rocket mass heater.
reply
    Bookmark Topic Watch Topic
  • New Topic