Incremental code review (ICR)
I suggested that we start doing weekly code reviews on two projects that I’m currently developing on. So we are on week three of one and week one of another. This is a break from the norm where we normally do the code reviews when we are done coding. I’ve been screaming for incremental code reviews for along time.
Some Do’s:
- Have the review at least once a week when coding heavy.
- Do include the designer or the business expert. (We have caught minor and major items before they became bugs/defects!)
- Everyone takes turns showing their code.
- Check the egos and the clubs at the door.
- Use a separate machine not the developers machine. You will be surprised at what you find or don’t.
- Make sure the machine has access to your CVS or whatever you are using. That way comments and code changes are checked in when you are done with the code review. Never let anyone take a note to change it later.
- Ask questions.
Benefits:
- Better team and code cohesion.
- Catching bugs early in the design.
- A true status of where we are.
- Pressure to keep the code in synch all the time.
That’s about it for right now. We have caught some holes in the design that would not have been caught until test. I think anyone can apply ICR to whatever methodology the subscribe to e.g. waterfall , agile … etc. We also seem to have a more complete earlier?
I guess I should add that one team has four and the other has three.
Don’t do the review if you don’t have the business people involved.
February 11th, 2008 at 4:43 pm
I’ve always said, if you wait until finished to review the code, it’s too late.
QA doesn’t want changes to anything at that point and the business owners don’t want to take the risk of changing something that works.
Now, to get QA involved at the beginning for incremental testing….
February 11th, 2008 at 8:56 pm
What I’ve found useful in the past is to have ad-hoc code reviews more often than that. We came about it accidentally the first time. We created a build system for our CVS repo and had it build whenever the repo was quiesced for 15 minutes (to give time to check in multiple files, fix merge conflicts, etc). Originally we sent mail only when the build broke, pointing to the most likely suspect in an effort to gently humiliate them and goad them to fix it. But one day someone suggested we also send emails for successful builds. Eventually we started diffing between builds and sending the entire patch as well as all the check-in comments as an email to the entire group.
These patch emails allowed incremental and continuous code reviews and let everyone on the project keep track of what everyone around them was doing. The emails got sent from the group so that you just replied to the patch and everyone got your comments. Generally we’d keep all critiques and praises public so that everyone could see the discussion–if they weren’t interested, they could just skip the email. We also made the email list very inclusive. We had all the hardware guys on our software list, for instance. When I asked them if all that extra email was annoying they all said, “No! We love seeing what you guys are doing.”
It was so successful that I’ve made sure to re-implement this for every project I’ve since worked on.
February 11th, 2008 at 9:29 pm
I don’t understand your final point. My personal experience has been that incremental code reviews are valuable and worthwhile without any business involvement. Care to elaborate further?
January 22nd, 2009 at 3:39 pm
IMO you need business people involved because they’re often the ones who define the product - they’re closer to the customer than the devs are, so they know better what the customer will like/dislike about the way a particular feature works or is implemented.
April 18th, 2009 at 4:40 am
Review of the code is important. But yeah, I think it’s more important to get the business people involved. I mean, I worked on teams where the developers have great communication with each other but form a cabal and pretty well tell the business people to buzz off until the project is finished. And then they worked on stuff that they think is important but not what is important to the business people, who may have a better idea of what the customer wants.
Hey, I’m just saying…