InfoQ

News

Design and Code Reviews : The Good, Bad and Ugly

Posted by Vikas Hazrati on Mar 07, 2008 07:02 AM

Community
Agile
Topics
Delivering Quality
Tags
Complementary Practices

In an interesting article on Design and Code reviews Kirk Knoernschild mentions that such reviews promise to improve software quality, ensure compliance with standards, and serve as a valuable teaching tool for developers. However, the way they are performed affects their value. In some organizations they might really add value to the software lifecycle whereas in others a review might just be a part of the bureaucracy.

He mentions some of the worst review practices as

Witch hunt reviews - Causing the developers who wrote the code to feel threatened and attacked.

Curly brace reviews - Emphasizing just on structure and indentation instead of serious issues.

Blind reviews - Reviewers have never looked at the code before and come to the review meeting unprepared.

Exclusionary reviews - Reviewing only a sample of the code and leaving out other important pieces.

Tree killer review - Waiting until the codebase becomes so large such that neither a complete review is possible nor effective.

Token review - Doing the review as a formality just because the management intends to get it done.

World review – Conducting the review infront of a huge audience, many of whom are not related to the project, there by intimidating the developers.

Kirk seems to mention that in order to do effective reviews; the team should try to automate the review process as much as possible and gather metrics. The team should try to incorporate feedback mechanisms in their existing development environment so that the developers are alerted about the red flags before they are ready to check in code.

He mentions some tools which help in bringing greater objectivity and focus to the review process, like

Kirk further mentions an interesting way to do reviews, called the 20% review

The idea behind the 20% review is simple: once 20% of development is complete, a review should be held. Some teams might find it beneficial to hold the 20% review each iteration. That's certainly effective, but I've found that if teams do a good job using metrics for a continuous review, holding the 20% review for each major system function is sufficient.

The 20% review should focus on initial design and code cleanliness. The metrics discussed above offer wonderful insight into the evolution and growth of the code while the size is still relatively manageable.

He concludes by emphasizing that using metrics to help drive reviews brings greater objectivity and focus to the review process.The greater the automation the easier it would be to arrive at those metrics and thus do an effective review. The review should also be held early enough so that the developers can utilize the learnings early on and the effectiveness of the review is not compromised.

6 comments

Reply

bad links by Sébastien Gruhier Posted Mar 7, 2008 7:53 AM
Re: bad links by Dave Rooney Posted Mar 7, 2008 8:48 AM
Re: bad links by Deborah Hartmann Posted Mar 7, 2008 12:13 PM
Code Reviews are good... by Dave Rooney Posted Mar 8, 2008 6:08 AM
Re: Code Reviews are good... by Vikas Hazrati Posted Mar 11, 2008 3:27 AM
Link icons by Ali Bolourian Posted Mar 11, 2008 8:56 AM
  1. Back to top

    bad links

    Mar 7, 2008 7:53 AM by Sébastien Gruhier

    FYI, You have some bad links in your article Seb

  2. Back to top

    Re: bad links

    Mar 7, 2008 8:48 AM by Dave Rooney

    The proper link is: http://www.agilejournal.com/articles/the-agile-developer/making-agile-reviews-effective.html Dave Rooney Mayford Technologies

  3. Back to top

    Re: bad links

    Mar 7, 2008 12:13 PM by Deborah Hartmann

    Thanks for your tact! Actually all the links were broken due to a formatting error. Fixed! Apologies for the inconvenience.

  4. Back to top

    Code Reviews are good...

    Mar 8, 2008 6:08 AM by Dave Rooney

    ...so why not do them in real-time? It's called Pair Programming! :) Dave Rooney Mayford Technologies

  5. Back to top

    Re: Code Reviews are good...

    Mar 11, 2008 3:27 AM by Vikas Hazrati

    Dave I totally agree with your statement. The problem is that in many companies, though they follow agile, pair programming has not really caught up as a result there is a need to do peer/formal code reviews later. I know a team of around 80 people split into smaller scrum teams who are not pair programming but they do want to wrote great code and do want all the team members to learn from the reviews. Vikas Hazrati http://vikashazrati.wordpress.com

  6. Back to top

    Link icons

    Mar 11, 2008 8:56 AM by Ali Bolourian

    Why do you use PDF icon for links at the bottom of your article? I saw this on other articles and other places on your web site too.

Exclusive Content

Getting Started with Grails

Jason Rudolph discusses Java/Grails integration, Grails plugins, creating a Grails sample application, Grails app structure, data querying and persistence, validation, controllers and tag libraries.

Creating Product Owner Success

The Scrum Product Owner role is powerful, valuable and challenging to implement. It brings healthier relationships between customers and developers, and competitive advantage - if you do it right.

Book Excerpt and Interview: Effective Java, Second Edition

Effective Java, Second Edition by Joshua Bloch is an updated version of the classic first edition, which won a 2001 Jolt Award. InfoQ asked Bloch questions about the areas that the new edition covers.

Tapestry for Nonbelievers

A new article by I. Drobiazko and R. Zubairov introduces v. 5 of the Apache Tapestry component-oriented web framework. The tutorial shows how to create a component and covers IoC in Tapestry and Ajax.

Pete Lacey on REST and Web Services

In this interview, Burton Group consultant Pete Lacey talks to Stefan Tilkov about his disillusionment with SOAP, his opinion on REST, and addresses some of the perceived shortcomings REST vs. WS-*.

Business Natural Languages Development in Ruby

Jay Fields presents his concept of Business Natural Languages - a type of Domain Specific Languages geared towards being readable by domain experts.

Distributed Version Control Systems: A Not-So-Quick Guide Through

Adoption and interest for Distributed Version Control Systems is constantly rising. We will introduce the concept of DVCS and have a look at 3 actors in the area: git, Mercurial and Bazaar.

Segundo Velasquez and Agile as Seen Through the Customer's Eyes

Deborah Hartmann interviewed Segundo Velasquez about his experience as customer with an Agile team during the initial phase of software design of a product.