Qualities of Code Reviewers

Question

When choosing code-reviewers for your project what qualities you see/prefer in the candidates (the one who does review)?

1. # of years of experience?
2. time availability (not selecting very busy guys)
3. one whose code gets least comment
4. senior junior ladder (juniors can't do seniors code review)
5. inside team or someone outsider
6. ...

Answer 1

It's been entirely too long since I was in an environment that did code reviews, but what I want in a reviewer is:

1. Someone whose judgment I trust.

That's pretty much it. If I know somebody else on/near the project can code well, I'm not particularly concerned about their years of experience or relationship to me on some kind of seniority chain; I want to hear what they have to say.

I'd also add optionally:

• Someone who understands the project -- but depending on the circumstances, a brief description of the piece I'm working on may suffice.
• Someone with the time to do the review right and not half-ass it to get it off his/her plate -- but a quick look from somebody who really knows their stuff can turn up problems/possibilities I missed.

Answer 2

Years of experience sounds good, but they are not. I met one guy who has over 15 years of experience. However he made some basic programming mistakes in his code (like hardcoding path to dir and doesnt caring that it wont work on other computers), something even junior programmer would think is wrong. maybe he was new to this language(he had only 5 years of Java experience), but he should have known that its bad and google the answer(or even ask, but he was too proud to ask ;).

I think he was typical career programmer, he doesnt want to waste time learning the language we were using(Java). However Im glad that I met him, so now I know that many people that interview me for a job are not smarter, just older.

I think you should peek someone who:

1) really knows and cares about the language (reading about it is a good sign).

2) wants to do it, forcing someone to do it is a bad idea.

3) knows how to design system, so later you don't have big abstract class hierarchy. but you need to watch out for design patterns junkie, he will design pattern everything.

4) is pro-testing (many people just don't care enough to get good at it). Plus its a lot of work and someone who doesn't like to code will never do it(if not forced).

5) is not closed minded. perfect argument is using final in method's arguments. its a subjective thing and I think it should not be forced on anybody(default checkstyle rules are forcing this, i think its bad because later you have two variables with almost the same name and its easy to make a mistake and you cant null the argument ;).

Only problem is that you can find performance junkie, guy that writes code so good that no-one can understand it. good sign here is using switch with fall-through, StringBuilder'ing every string concatation(that is bad, because compiler already does it in many cases and it makes the code look bad), etc. That guy will focus only on performance and not performance that matters, but useless performance. i bet you he will never optimize SQL(except replacing * with columns names, because he will do that even if its not needed).

Answer 3

My biggest two items are:

1. Domain expertise. It doesn't matter much if your code is correct if it's not actually doing what's needed in the domain. Sometimes bugs or requirements show up that the domain expert can call into question and prevent you from either doing the wrong thing or wasting time fixing a non-issue.
2. Willingness to review. Some people just won't take the time. You want someone willing to be critical and take a hard look at the code. It makes the process more painful sometimes, but a poor code review is really just a waste of time.

Answer 4

We use fisheye for doing code reviews. It has a neat feature called 'Suggest Reviewers'. Basically what it does is suggests the committers who modified the same files that are under review. I find this very useful and use it all the time.
So to answer your question about the qualities of code reviewers:

• Familiarity with code under review
• Any other team members that you respect from a technical perspective

Answer 5

Anyone and everyone on the team. As long as he/she takes the job seriously (and if not, s/he shouldn't be on the team).

The company that I think did the best job of code reviews used a four-person, one-hour format: the person whose code was to be reviewed, at least one senior team member to provide overall "how does this fit into what we're doing" critique, and at least one junior member, who was expected to learn something as a result. The junior team members usually found the problems, because they spent the most time trying to understand and analyze the code.

The stated goal was to have at least 2 reviews a week, with different members for each review. Members were expected to put in another hour preparing for the review (which I think kept the reviews from degenerating into comments on coding style).

Given the level of focus, these review sessions couldn't possibly cover the entire codebase. Instead, the goal was to pull out the 10% of the codebase that is likely to cause problems; after all, most code is simply boilerplate.

Answer 6

• Breadth of experience matters far more than time served. If someone has spent 20 years writing in only one language then they are likely to be extremely picky about irrelevant details, whereas someone who has seen thread bugs in multiple languages is much more likely to see through to the core of issues.
• Relationship with the programmer and the code -- they need to care about getting the code right, and they need to have a constructive working relationship with the programmer.
• Incentives must be correctly set -- reviewers must not feel that time spent reviewing will diminish perception of how busy they are, or they may rush it.
• Full system visibility so that they understand the real purpose of the code. They should be able to review the code not just for correctness against a specification, but in terms of its overall place in a project. This determines the relative priorities of things like performance and reusability.