December 29, 2008, 9:39 AM — Code reviews are expensive. Time spent reviewing code by managers and peers is time spent not programming. So if you're going to do code reviews, it makes sense to do them well. That's true of the process itself (see Running an Effective Code Review) and the actual line items to which you pay attention during the review.
But the first order of business is to identify who should lead the code review, and who else ought to participate in the code's evaluation. The first answer is fairly clear, but as you'll see, developers have wide-ranging opinions about the second.
Straight-up: the person running the code review needs to be a technical authority and ideally a warm and supportive leader. Micheal Lalande, director of technology at QLogitek, says, "Peer reviews are great, but if the people doing the review are less competent than the person doing the coding, there is little to no value other than possibly training a less experienced developer."
Expertise is important, whether it comes from managerial responsibility (i.e. the project lead) or the subject matter of the code being examined. "The best code reviews were ones where the code reviewer was someone of stature, well known in that particular area of development," says Lalande. "They worked closely with the development team to ensure they understand the context within which the software had been developed, and can work with the developers to understand why particular development decisions were made."
In addition to the leader being a senior engineer, she must be given authority to do code reviews from the engineering department head (Director or VP), says Christopher Buchino, director of software engineering at GotVMail Communications. "This person should have intimate knowledge of established coding standards as well as a mastery of software development best practices."
Another thing to look for, says Tim Rosenblatt, the Agile development director at Cloudspace, is someone who has seen many different coding styles. "There are a lot of people who write code very well, but will be overly (and unnecessarily) critical of code that has a different style from their own," he points out. "It's good for the reviewer to be able to objectively explain why the advice they are giving is correct. Different projects may have different requirements, so having an objective explanation helps you decide if the advice fits in with the project requirements."
But don't underplay soft skills. Marc Carkeek, VP of development and support at UC4 Software, a provider of workload automation and IT process optimization solutions, says, "You need a strong mediator to ask questions and prompt discussion, while crafting the message so the person whose code is being reviewed isn't offended."
Theron Welch, software mentor at the Microsoft Asia Center for Hardware, says, "An independent moderator can keep the discussion from getting too personal. A good moderator is someone good at running meetings, keeping people on track and focusing on important issues."
Peer Reviews Versus Code Reviews
Some developers may disagree with the above, believing that code reviews should never have a "boss" in the room, that all such meetings should be peer reviews.
Jay S. Hemmady, a former technology chief who is now working at a stealth eCommerce portal, says coders' managers might not want to attend if the meeting is meant to be a true peer-review. It's hard to avoid the feeling that a person's performance is being measured if a manager is present, he points out. "In larger organizations, this is easy to accomplish without managers. In smaller ones, often the manager is the more experienced programmer/coder!" he adds.
Yet, advises E. William Horne, systems architect at William Warren Consulting, the boss should sit through a code review if possible. "She might spot hidden agendas, grandstanding, wishful thinking or feigned interest that you may miss because your head is inside the machine," Horne says.
Scott Butler, a senior sales engineer at Servoy USA, has seen some departments that have the same developers that write the code, doing each other's code review. "If you have a very good experienced, small team, then this is fine," he says. "However, you may have a team of developers with varying skills. Sometimes the initial reaction is to still have everyone participate in code review so that you don't hurt anyone's feelings. This is a mistake. Code review should be done by your best programmers or analysts."
So much for consensus. Because "who else ought to be in the room" generates widely varying opinions.
Who Should Participate
The participants' roles depend, to some degree, on the nature and purpose of the code review and the experience of the people involved.
Some people feel that code reviews are inherently one-on-one mentoring sessions, so the proper participants are a senior developer or team lead and the (usually junior) developer who reports to him. Most, however, expect that the code review involves bringing several people into a room. The key is balance-whatever it takes to get it.
For EMC distinguished engineer Steve Todd, an ideally balanced code review team consists of a hands-on software developer (typically a peer of the contributor), an architect (who is aware of the software' future direction) and someone in the middle (typically a project lead) who constantly has to weigh the compromises between time-to-market and the future needs of the software.
"I have seen some very good decisions result when this balance is present," Todd says. "For example, programmers have used static, hard-coded data structures (e.g. arrays) to efficiently store small amounts of information. The software contained no bugs and was well written. There were no issues from the project leader or the peer software developers. However, the architect pointed out that the future product would need to be much more scalable, and suggested abstractions layers to 'hide' that a static array was being used. This resulted in much more extensible software, even though the code review found no defects. Balance is a key aspect of code reviews."
That balance can come from outside the core team, too. It can also help to include reviewers from different development teams, according to Manoranjan (Mano) Paul, the software assurance advisor for (ISC)2, "so that a different perspective and ease of understanding of the code (for maintainability) is determined in addition to determining coding issues."
That viewpoint has quite a bit of support. Says Jeffrey Henning of PRC Vovici, "The best reviews have been when we have cross-functional representation, so that we have a UI person, a DBA, the programmers, and everyone has come prepared, everyone has looked at the code." Henning points out that the UI specialist might have an insight the coder didn't have; the DBA might have a performance improvement in mind. "As a result, we get the benefit of all these different perspectives."
So who shouldn't be there? For some, it's important to have a representative from the business. For others... not so much. "Active members should all be coders who have a role to play," says Hemmady. "Non-technical people who don't understand code or the development process can at best be non-vocal participants. But watch out for such people to 'bad-mouth' the process outside the team afterwards."
Another issue is the involvement of the software quality assurance (SQA) professionals in the code review. Some feel that SQA ought to participate, at least from the perspective of ensuring that the code is test-ready from the get-go. But Horne says, "Do not, under any circumstances, invite the testing team to a code review. Their job is to make sure that your code doesn't break anything that's already in production, and to make sure that it does what it's supposed to: Code review is about how to get the best result, not what that result will be."
Testers must work with "black boxes" in between their inputs and their result sets, Horne points out. "That's their job, and forcing them to sit through what is, to them, an unimportant discussion about something they can't change can only make enemies you don't need," he believes.
In some cases, though, the code review participants you truly want may not have time for the meeting. As senior software engineer Alexei Zheglov says, "Potential reviewers should be at above-average experience levels, but they tend to have above-average responsibilities and therefore below-average availability."
