Recently I was reminded that I used to be a proponent of code review...
I am (still) a big fan of code review. But code review only works under certain circumstances, and I think those circumstances are hard to find at Unidata.
- There must be 3 or more reviewers (in addition to programmer who wrote the code). They must all be programmers in daily practice with the same language and environment as the reviewed code, and working on the same project.
- Code is judged against written, reviewed requirements.
- Code that satisfies requirements with least number of functions, variables, lines of code (within reason) is best. (Occum's Razor for code.)
- Must be full buy-in from the programmers and all their management. This is a very expensive process.
- All released code (except test code) must be reviewed.
- Major (user-affecting) and minor (bad coding) defects are identified but not fixed at review. There is no discussion about potential fixes - a problem is identified and the review moves on.
- All major and minor problems must be fixed by original programmer, and the code re-submitted for re-review. (Usually much quicker, but still necessary.)
- Review should be one hour at a time, with one or two hours more mandatory preparation by all reviewers.
- Records are kept, project manager follows-up.
- Unanimous decision required for all defects and to pass review.
- No supervisors or spectators ever.
As you can see, to meet all these conditions is no small feat. In fact, I have hardly ever seen it done. When I have seen it done, it has worked very well. Ideally, it becomes a place where all the project programmers learn from each other. The best practices spread through the project code, and bad practices wither away.
How can this work at Unidata? We don't have the programmers!If we got more programmers, I would still think that other, more inexpensive software process improvements should take place first (like written requirements, and requirements review).
However, I would be willing to participate in any code review that anyone organizes at Unidata as long as two conditions are met:
- It seems to be part of a serious effort - not just a casual thing. There must be feedback of review into product. (That is, something must be done with results of the review.) And there must be written requirements so I know what the code is supposed to do.
- My project(s) must be compensated in some way with time. I need someone else to do that quarter-day a week of work I would give up to have time for review. For example, I would do plenty of good reviewing for anyone who answered 3 netCDF support questions a week!
Most of your list looks good, but there are some things with which I disagree:
* There must be 3 or more reviewers
Our research has shown that reviews with only 1 or 2 reviewers can also be very worthwhile.
* Code is judged against written, reviewed requirements. Ideally, yes. But even if there are no written and reviewed requirements, you can still get value from doing code review. As long as the requirement is understood by the reviewer(s) (whether it is written or not) then they can validate that the code produces the correct result. Even if the actual business requirements are not known, a reviewer can still comment on the maintainability and scalability of the code.
* All released code (except test code) must be reviewed. This is where I have the strongest objection. Trying to do an "all or nothing" approach, unfortunately, usually ends up with nothing. It's better to start slowly - with just a subset of the code (e.g. the stable branch, core modules, "scariest" files, etc.) and let people have time to adjust to the process change. Further, unit test code is one of the most *interesting* things to review - if the unit tests are passing but they are wrong and/or incomplete, then how useful are they, really?
* Review should be one hour at a time, with one or two hours more mandatory preparation by all reviewers. That sort of sounds like you view this as two phase - the reviewers work independently to prepare and then come together for a meeting that lasts one hour. That's one approach, but not the only approach. Code review without meetings can also be effective - more details in our book: http://codereviewbook.com
Posted by Gregg Sporar on January 11, 2010 at 02:36 PM MST #