posted 07-26-2002 02:56 AM         

------------------
Dinesh

posted 07-26-2002 06:23 AM         

1) Reviewers who don't have enough experience to be effective

posted 07-26-2002 06:40 AM           

My experience is that people do not actually look for problems in some cases. They simply go through the code as if they were reading a book and not actually thinking about what the code is doing. It is easy to get into what is called the "flow experience" in group psychology and this definitely makes strict code reviews less effective.

Another problem is that the meetings for the reviews are held and people are not prepared. They have not looked over the material prior to the review and thus are not only trying to be helpful but also trying to come up to speed at the same time. The "coming up to speed" part should have been done prior to the meeting. When there is a lot of catching-up during the review, this can make them less effective than they otherwise would be.

Another problem is that a set of rules are not established for what to look for and, thus, the review is done largely ad hoc. A good code review checklist should be present in the meeting and should be followed so that a variety of problems are kept in mind and looked for. When this is not done, it is quite possible for a review to seem effective but, in fact, to miss a lot - thus making it ineffective.

Also the point sometimes gets lost in these reviews and they become debates about whose coding style is "right" or "wrong" or who was right or wrong to code a routine in a given way. The idea gets lost that the idea is to find defects in the code. Granted, part of the review is looking for the adherence to coding standards but one has to realize these are, by nature, critical meetings ("critical" in the sense of criticism) and thus some people skills come into play here in terms of focusing on the issue and not the person.

A sort of "group think" can pervade a review group if the developers are sort of "on the same page" with each other, so to speak. In other words, you might have a group full of "yes-people" who will simply go with what is there because they feel that their colleagues wrote it and so it is good enough. This is different, in kind, from the "flow experience" problem. It is important to have people who are willing to be critical of the overall work. (That is where having the "rules" of the review in place really helps because it forces people to not so much concentrate on the "work of such-and-such person" but rather just as an abstract example of the implementation of a given rule.)

I would also second Joe's idea: if the reviewers present for the code review do not have the necessary expertise, it is quite obvious that the review will not be the most effective. (This is the simplest problem to state and is the most obvious but it certainly bears mention.)

posted 07-26-2002 07:40 AM         

I also recommend picking up a copy of "Handbook of Walkthroughs, Inspections, and Technical Reviews" by Daniel Freedman and Gerald Weinberg. Daniel also teaches a class based on the book. I've sat through it twice and think it's great!

posted 07-27-2002 02:44 AM         

------------------
Dinesh

posted 07-29-2002 05:36 AM         

Imperfect moderating. To my mind, this is the single biggest reason that reviews are ineffective. The moderator is a key role in reviews and if the moderator is not skilled, then the whole review can be a disaster. The symptoms of a poor moderator are mostly those shown in the other answers to the question. For example, I have made myself very unpopular, as a moderator, by throwing a reviewer out of the review meeting when they have not come prepared – I don’t want that person, they are going to prolong the meeting and give sub-standard comments. Too few people are trained in and skilled in moderating techniques.

The wrong people in the review. The people need to have the right skill-set and right mind-set. Too often a Manager who hasn’t written a line of code in years will attend a code review, they are the wrong person, you want people who know how to code and will constructively criticise.

Too many people at the review. Somewhere between 4 and 6 people is optimal. The length of the review goes up exponentially as the number of people increases and the average quality of the comments goes down.

Review too soon. If the code is not ready to review, but the date on the plan says “send code out for review dd/mm/yy”. This causes a lot of review comments that should not be made, as they would never have got to review if the code had been reviewed when it really is ready.

No follow-up on actions. The review meeting is not the end of the review, all actions must be followed up on.

Review out of context. Reviewers cannot just review a piece of code, they need to know at least what the design is and how it fits into the rest of the system.

Reviewing too much. Code reviews are time consuming. Reviewing every module may not be cost effective, so review those code modules that will get most benefit from a review.

Using review comments as a measure of how good the developer is. A complete No-no. A good way to make your developers leave the company.