Meaningless Drivel is fun!*
The moose likes Agile and Other Processes and the fly likes [Ship It!] Code Reviews Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Engineering » Agile and Other Processes
Bookmark "[Ship It!] Code Reviews" Watch "[Ship It!] Code Reviews" New topic
Author

[Ship It!] Code Reviews

Jason Menard
Sheriff

Joined: Nov 09, 2000
Posts: 6450
I have worked in a very process heavy organization (sprechen zie CMMI?). One of their core processes was code reviews. As such, this is not just a case where one developer hands his code to another and asks them to look over it, it's a major undertaking. Their process states that 25% of code will be reviewed. Code reviews are carried out a few times during development.

Let me explain the process that is undertaken for every code review...

1. Kickoff meeting. Explain what code is being reviewed, what is being looked for, who the owners of the reviewed code are, and who is responsible for reviewing what code. Introduce the moderator (yes, code reviews are moderated) and the scribe (the person who logs defects in the defect logging meeting). Give date for defect logging meeting, by which all code assigned code must be reviewed.

2. Code reviews. Developers go off and review their assigned code. We have a document describing what we are inspecting code for, with heavy emphasis on style and comments. When a defect is found, it is logged on a defect logging form giving the line numbers in the code, a description of the defect (the process actually says to state this in 7-10 words), a severity of the defect (question, minor, major, super major), and the category of defect (missing data, erroneous data, standards, and a couple other things which don't have much relevance to code). Reviewers must keep track of how much time they spent reviewing each file and note it on the defect logging form.

3. Defect Logging Meeting (DLM). We go through each piece of code being reviewed. The moderator asks each person reviewing the current code under discussion to read one defect, and the scribe adds it to a master defect logging sheet. This round robin approach continues until each person has read all of their defects. Each piece of code is reviewed in this manner. The DLM concludes with an opporunity to give comments on the process, which the scibe dutifully makes note of.

4. Code rework. Code owners are to look at the master defect logging forms for their code and either fix the errors found or reply as to why they weren't errors, naturally keeping track of how long this rework takes.

5. If necessary, code is reviewed again after changes are made and we have another DLM. Truthfully, we try hard to avoid this step.

I should note that the process actually derives from a process of reviewing documents, and not reviewing code. I should also note that this process is not driven by the software developers, but rather by the organization's "process engineering group".

What might your book have to say about such a process for code reviews?
Kishore Dandu
Ranch Hand

Joined: Jul 10, 2001
Posts: 1934
The above looks good for long term project. But for short term or immediate deliverable projects(which are norm in web realm) this is a overall, in my personal opinion.

This is what we follow.

1. swap the code and requirements to a different developer who has zero knowledge of the feature.
2. Log all the observations and fixed done to address those observations.
3. We also log changes done due to regression and initial couple of rounds of QA testing.


Kishore
SCJP, blog
Jared Richardson
author
Ranch Hand

Joined: Jun 22, 2005
Posts: 113
What might your book have to say about such a process for code reviews?


Directly from Ship It!


Small, frequent code reviews keep your code clean, simple, and tidy.
You can avoid the traditionally unpleasant code reviews that involve
dozens of developers and require days of preparation (a.k.a. The Mighty
Awful and Dreaded Code Review, hereafter referred to as MAD reviews
for your reading enjoyment). We�ve found code reviews can be painless
when you adhere to the following rules:
  • Only review a small amount of code.
  • There are one or two reviewers at most.
  • Review very frequently, often several times a day.



  • The book goes into a bit more detail, but I think you get the idea. Storing up large portions of code for a review practically guarantees that the review will be a huge time sink and also cover so much information that it will impossible to catch all the problems.

    On the other hand, if you have no choice but to do the CMM reviews, these two types of reviews are not mutally exclusive. You can still do the mini-code reviews as you go. In your situation this would make the MAD reviews much more useful because more than one person would already be familiar with the code in the big review.

    I really don't like MAD reviews, but if they are forced on you, try doing the mini-reviews as you go. Then when you sleep through the MAD review, you can still feel good about the code you ship.


    Check out <b>Ship It! A Practical Guide to Shipping Software</b><br /> <br /><a href="http://www.pragmaticprogrammer.com/titles/prj/" target="_blank" rel="nofollow">http://www.pragmaticprogrammer.com/titles/prj/</a>
    Will Gwaltney
    Author
    Greenhorn

    Joined: Jun 24, 2005
    Posts: 9
    Originally posted by Jason Menard:

    What might your book have to say about such a process for code reviews?


    Oh my. Not to put too fine a point on it, we condemn such things in the Strongest Possible Terms. The code reviews we talk about are fast, informal, frequent (at least daily), and (relatively) painless. We suggest no more than one or two reviewers at the most, and we let the source code management system do the documenting by listing the reviewer's name when code is checked in. That way, developers spend more time coding and less time acting as the "code police".

    If it turns out that a developer needs as much guidance as your process suggests, we advocate assigning them a mentor to help them along. Otherwise, get out of the developers' way and let them code.


    Check out Ship It! A Practical Guide to Shipping Software<br /> <br /><a href="http://www.pragmaticprogrammer.com/titles/prj/" target="_blank" rel="nofollow">http://www.pragmaticprogrammer.com/titles/prj/</a>
    Jason Menard
    Sheriff

    Joined: Nov 09, 2000
    Posts: 6450
    Some good feedback, thanks. I certainly agree that the small frequent reviews are the way to get the most benefit. Is there any method you use to figure these code reviews into the schedule?
    Jared Richardson
    author
    Ranch Hand

    Joined: Jun 22, 2005
    Posts: 113
    Originally posted by Jason Menard:
    Some good feedback, thanks. I certainly agree that the small frequent reviews are the way to get the most benefit. Is there any method you use to figure these code reviews into the schedule?


    These reviews might take some time for the first week or two, but after that they fade into the background (in terms of time). When things are going well a code review will take 5 to 15 minutes, not enough time to bother scheduling. When we say small and frequent, we mean it!

    To frame this a bit more... when you have code you are changing or adding, it should be in response to a specific feature request or bug report. Hopefully these issues are on your copy of The List. When you complete an item from The List (a single feature, part of a feature, a single bug, etc), then stop, get a review, and commit the code into your source code system.

    What does this buy us?

    First, when others are using the source code management history features, they'll have smaller commits to look at. You haven't committed a weekly "code bomb". You've committed a patch to fix bug X.

    Second, you have a small, manageable chunk of code to review. It's not 17 bug fixes ranging from the database to the GUI. It's a single bug that (hopefully) you and the reviewer can understand.

    Third, you're isolating your changes. With a Continuous Integration system constantly compiling and testing the code, you'll know exactly which code changes broke the compile or the tests. Since you know exactly what changed (and you got feedback 20 minutes after you committed the code), you can fix the problem quickly.

    Once you get in the habit of quick, small code commits you'll be horrified at how hard it is to deal with weekly or monthly code bombs.
    [ August 03, 2005: Message edited by: Jared Richardson ]
    Jeanne Boyarsky
    author & internet detective
    Marshal

    Joined: May 26, 2003
    Posts: 30938
        
    158

    Originally posted by Jared Richardson:
    When things are going well a code review will take 5 to 15 minutes, not enough time to bother scheduling. When we say small and frequent, we mean it!

    Does that include pseudo code reviews that people do as they pull in code from the repository or look at it for other reasons? These tend to be below the level of what is called a code review, yet there is certainly code reviewing going on.


    [Blog] [JavaRanch FAQ] [How To Ask Questions The Smart Way] [Book Promos]
    Blogging on Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, OCAJP, OCPJP beta, TOGAF part 1 and part 2
    Jared Richardson
    author
    Ranch Hand

    Joined: Jun 22, 2005
    Posts: 113
    Originally posted by Jeanne Boyarsky:

    Does that include pseudo code reviews that people do as they pull in code from the repository or look at it for other reasons? These tend to be below the level of what is called a code review, yet there is certainly code reviewing going on.


    The code reviews in the book specifically talks about the interaction between two (or more) developers. A big benefit of the review process is the knowledge sharing that goes on. One developer showing others what he did, explaining his intentions, sharing stylistic conventions, etc.

    There's certainly nothing wrong with learing from code from the repository (or Google!), but I'm not sure I'd call it a code review.
    Tony William
    Ranch Hand

    Joined: Jun 27, 2005
    Posts: 54
    Just to share my experience in doing code review.

    In the past year, a new team of 7 is formed with most of the developers new to the organization (& hence new to our internal application framework for web application development). A few of them are also new graduates.

    As the team lead, I alone is the one doing small frequent code reviews at the beginning. Then later on, I have another more experience staff helping me..... Now (after 1 year), most of the members in the team get used to it in reviewing code from others. Even not being told, they will voice out to each other if they spot something wrong.... Well, in most cases, I would say the code review is done for QA purposes (I am not sure if this is code review anymore)

    I find this a good way to share the knowledge & build up the skills in the team.


    MCP, MCP+I, MCSE(NT4), MCSE+I, MCSE(2000), MCDBA, MCSD(VS6)<br />SCJP 5.0, SCBCD 1.3<br />ICED(v5.0), ICSD (WSP5.0)
    Jared Richardson
    author
    Ranch Hand

    Joined: Jun 22, 2005
    Posts: 113
    Originally posted by Tony William:
    Just to share my experience in doing code review.
    As the team lead, I alone is the one doing small frequent code reviews at the beginning. Then later on, I have another more experience staff helping me..... Now (after 1 year), most of the members in the team get used to it in reviewing code from others. Even not being told, they will voice out to each other if they spot something wrong.... Well, in most cases, I would say the code review is done for QA purposes (I am not sure if this is code review anymore)

    I find this a good way to share the knowledge & build up the skills in the team.


    Absolutely! Are you sure you haven't read Ship It?

    From pages 93 and 94 of Ship It!


    When you introduce the code review process, you may need to appoint
    a few senior team members to be the mandatory reviewers; one of
    the senior team members must participate in every review at first.
    You shouldn�t need them to continue in this role for more than a few
    months. Once your team members learn the basics, the whole team
    will be capable of sharing the responsibility. As the proverb says, �As
    iron sharpens iron, so one man sharpens another.�9 The point is for
    the team members to work together and so improve each other. Involve
    your team members in the sharpening process as quickly as possible.

    We worked in one shop that really illustrates how code reviews can
    be used to leverage your senior members. We had three very senior
    developers and five who were decidedly not�they weren�t rank novices,
    but sometimes they had peculiar ideas of how to fix a problem. In order
    to protect the product and to bring the junior developers up to the next
    level, all code reviews involved one of the senior team members. This let
    the more experienced team members instruct and teach while catching
    problems before they were introduced into the product. It also made
    the senior team members aware of misunderstandings and real issues
    that the junior developers faced.

    These reviews were a great help to the team. We frequently spotted
    repeated code and summarily pulled it out and moved it into utility
    classes. Reviewers caught and removed code that had nothing to
    do with assigned work (otherwise known as freelance refactoring) and
    rejected uncommented code outright. As the team moved forward, an
    imperceptible (but very important) change took place.

    Each of the junior team members started picking up good habits, one
    code review at a time. They started cleaning up code before the reviews,
    adding meaningful variable names, comments, and such before they
    were asked. Long, cumbersome routines became short and manageable.

    Even better, the lessons taught in the code reviews stuck. After about
    three months, we changed the code review policy so that any team
    member could do the reviews.

    [ August 03, 2005: Message edited by: Jared Richardson ]
    Tony William
    Ranch Hand

    Joined: Jun 27, 2005
    Posts: 54
    Jared,

    I am sure that I haven't read the book.

    After reading the difficult threads and with the information that you provided, I believe the book should be a good reference, with lots of ideas or insight that built on top of experience & concrete ground.
    Rudy Harianto
    Ranch Hand

    Joined: Dec 01, 2003
    Posts: 94
    Jared,
    This was taken from the intro from your book:


    And once the review is over,
    show the rest of your team what you�ve done with code change notifications.


    I wanna know what kind of notifications does it means? through email or oral? How important is this notifications?
    Cause from your suggestion that we should review just a small amount of code, so wouldn't it be a lot of notifications that should be made?


    SCJP 1.4, SCWCD 1.4, SCBCD 1.3, SCJA<br /> <br />blog: <a href="http://jroller.com/page/rharianto" target="_blank" rel="nofollow">http://jroller.com/page/rharianto</a>
    Jared Richardson
    author
    Ranch Hand

    Joined: Jun 22, 2005
    Posts: 113
    Originally posted by Rudy Harianto:
    Jared,
    This was taken from the intro from your book:



    I wanna know what kind of notifications does it means? through email or oral? How important is this notifications?
    Cause from your suggestion that we should review just a small amount of code, so wouldn't it be a lot of notifications that should be made?


    From Ship It!, page 98


    When you edit code, an automatic build system can notice the change
    and rebuild the project (see Practice 4, Build Automatically,, on page 30).
    Your next step is to publish that information so that every member of
    the team knows what changed.

    A change notification system pushes this information out to your entire
    shop, not just your immediate co-workers. The effect from this type of
    knowledge sharing can be quite amazing.

    Similar to Alistair Cockburn�s �information radiators,� you are making
    information available. Your team can use it or ignore it, but the information
    is being put out there for you. In fact, this practice doesn�t
    make you fetch the data; it pushes it to your desktop.

    Each time we�ve introduced this practice, a sizable percentage of the
    shop has been opposed to the practice before they�ve tried it.

    After about a month, the worst complainers always come by to apologize
    and tell us how useful the tool has become to them. This technique
    consistently brings the most resistance, but after a short time everyone
    becomes acclimated to having the notifications available. It quickly
    becomes a vital resource.


    How's that?

    Any of the Continuous Integration systems will send out email to notify everyone of changes (or not... they're configurable). This can be a lot of email, but it's easily scripted into a sub-folder.

    And of course, when there are a lot of changes, it's in your best interests to be aware of those changes as they occur instead of getting surprised later.

    If you shop isn't using a Continuous Integration system you can send out diffs manually (I still like email), but automated is better.
    [ August 03, 2005: Message edited by: Jared Richardson ]
    Jeanne Boyarsky
    author & internet detective
    Marshal

    Joined: May 26, 2003
    Posts: 30938
        
    158

    Originally posted by Jared Richardson:
    The code reviews in the book specifically talks about the interaction between two (or more) developers. A big benefit of the review process is the knowledge sharing that goes on. One developer showing others what he did, explaining his intentions, sharing stylistic conventions, etc.

    There's certainly nothing wrong with learing from code from the repository (or Google!), but I'm not sure I'd call it a code review.

    There is interaction, but it's more one way. The person who notices stuff points it out to the developer, which often leads to a discussion. I agree that more formal discussion is probably more in line with a code review though.
    Tony William
    Ranch Hand

    Joined: Jun 27, 2005
    Posts: 54
    For the informal discussion among 2 developers, I have heard someone else using the term "peer review". From my understanding, this is somehow in the middle of "friendly chat among 2" and "formal discussion / meeting among the team".
     
    Don't get me started about those stupid light bulbs.
     
    subject: [Ship It!] Code Reviews