In general, I’m not a huge fan of code reviews but they can be an effective tool if they’re kept in check. A co-worker asked me to put together some talking points about code reviews, which I did, regarding some of the things I thought would be material to keep in mind for a code review process. I thought they might be generally useful, so I’m posting them here.
- In general, I’m not a big fan of invasive, comprehensive code reviews. They tend to have diminished value the more comprehensive they are. I AM a fan of spot checks and quick audits concentrated mainly on those developers that maybe unknown quantities, e.g. new contractors, new team members etc.
- AUTOMATE whenever and wherever possible. If style standards or code test coverage can be automated (which it can), it should. Tools like CheckStyle and others are invaluable and are huge time savers. They also have the benefit of being impersonal. Nobody gets offended if a tools yells at you. Before anything’s done manually, ask “Can this be automated?” Manual review *should be* the exception, not the rule…
- If there are formal code reviews, the purpose and the criteria that is going to be checked should be explicitly stated up-front. I’ve seen too many code review sessions spiral into arguments that essentially boil down to “this is NOT how *I* would have implemented it!” as opposed to having a constructive discussion that ultimately adds value to the process.
- Code efficiency can be a dicey topic. I’ve seen a lot of developers spend hours arguing about the efficiency of a piece of code that might run once per day. A strong moderator that has good mediation skills is important. The reviewers must be mature enough to know when to pick a fight and when good enough is good enough.
- A good process with well defined roles goes a long way to alleviating arguments and maximizing the value of code reviews. In terms of roles, I would suggest: 1) Moderator 2) Peer 3) Developer who is being checked. The peer reviews and then the three people get together in a meeting and review the results. The moderator mediates and keeps the meeting rolling along and notes any action that is required to be taken. I would suggest a time and scope limited review session. Around 1-1.5 hours seems to work with follow-up sessions as needed. Any longer and eyes glaze over and people’s interest fads…
- Don’t let perfect get in the way of good enough – again there has to be a good balance and generally this discretion falls to the moderator, hence the moderator should be somebody who has good facilitator skills.
No TweetBacks yet. (Be the first to Tweet this post)
{ 2 comments… read them below or add one }
That’s a good list, especially the part about not spiraling into an endless discussion. FWIW, a while back our company did a study of code review at Cisco – the results are available in a book that we make available for free on our web site: http://smartbear.com/codecollab-code-review-book.php
I have three questions I subject activities to: does it share knowledge, create teamwork and increase efficiency. Reviews meet that criteria. I like to spend most effort on designs and design reviews, less on code reviews. I am a big fan of peer reviews for code. During the design phase complicated areas can be identified and targeted for code review and magnitude of review. Also during the design phase, modules targeted for reuse can be identified and given the extra time and quality warranted, implying other code should not be overly designed.
Steve Mixon mixonsr@yahoo.com