Who should perform code review?

Question

I have heard two opposite views on this:

1. It should be done by the people who know the overall architecture but doesn't know the intricacies of that particular component (i.e. people from same project but with different component). The argument here they don't have any preconceived notions about how things work and may be able to take a fresh view of the code.
2. It should be done by the people who know in and out of that component. This is because they know the history of the code like why particular piece of code is written in a certain way and may be better handled in anticipating the problem. But since they know all about the component, they may assume certain things.

I guess it would be ideal to have both types of people. But if I assume that due to some reason I can have only one type of review, which one may be more productive? What is your experience about it?

Answer 1

My philosophy

As many people as possible

Experience and/or knowledge of a particular is not always the determining factor in who does the best review. I've seen as many novices as experts catch big issues in code reviews.

Answer 2

Hi, the best results (increase in quality of code) with code review, our team had with the people which didn't have close connection to the code.

As You've said, they bring the fresh point of view, and can show You the issues which the regular team could not see.

Too many people during code review can bring to big mess. Everything needs to be balanced.

Answer 3

I vote for Number 2.

The idea of having a code review is making the code 100% comply with the requirements\standards\conventions\best practices. If type 1 guy is doing the review he can only concern about the standards\conventions but he cannot assure the compliance to the requirement and perhaps the program logic.

Answer 4

The best choice probably depends on the current phase of the project.

In the early stages of the project, overall understanding of the architecture is paramount. Many of the developers probably don't have a solid understanding of the architecture and project's best-practice patterns. It's the right time to invest the time of someone who does to ensure that knowledge is spread throughout the team.

If you're in maintenance mode, individual changes are likely to be fairly localized, and most developers should have a good understanding of the architecture by now. It may be best to have someone review the code who has a good grasp of the individual component.

Answer 5

Where I work, the code reviewer must meet two criteria:

• Someone familiar with the style guidelines we have for the particular language you are programming in.
• An "owner" of the location you are checking the code into.

One person can do the code review (that meets both of the criteria). The idea is that you have someone making sure you are adhering the style guidelines, and that you have someone who is familiar with the code your change is directly affecting. That way they can give you good insight into "the way things are done" and give you foresight into what you might be affecting.

Also, other people familiar with the project are encouraged to look at your code and add comments. We always do. If I notice someone is submitting code that I'm interested in looking at... I look at it. It's a good time to bring up that "other way" you were thinking of doing and ask people which way they think is "better". They often give some good insight as to why you should do it one way over the other for the specific case you are dealing with.

Just try not to let too many people look - because too many chef's spoil the broth. (I think people know when they shouldn't be giving their input). To give you an idea... I think the most people that have ever reviewed one piece of my code is ~5. And normally there are ~2.

Having style guidelines makes sure you don't get into style arguments in code review. And the reviewers should have some familiarity with the different components that interact. I never do code reviews outside of the components that I work on.

Answer 6

I think that people that do not have a close, lets call it relationship to the code reviewed bring some kind of fresh mind into the review itself. They may want to know why a component is designed in a specific way, even if the component itself is not the scope of the review. They may need to get the big picture before staring the review. So maybe they can detect special design flaws.

Answer 7

It should be done by someone who may have to maintain the code in the future:

• They have a more-than-only-theoretical interest
• It's an opportunity for knowledge transfer ("Why did you do it like this?")
• It's a form of acceptance testing ("Yes, if you just change this and that, I accept that I would understand the rest of it well enough to maintain it")

Answer 8

First you have to decide what the purpose of the code review is. Here are the things I look for, in my priority order:

1. Learning. The code review is a good time to discuss why the code was written this way, and what alternatives are available. Both the original coder and the reviewer contribute ideas to this discussion. The goal isn't to change the code, but to change the people.
2. Consistency. Ensure consistency of coding practices from formatting to idioms to sharing existing code.
3. Redundancy. Make sure more than one person is familiar with every bit of the codebase.
4. Find mistakes. When you explain your code to someone else, it forces you to think through it in a different way, and you're likely to find your own mistakes in the process.
5. Find mistakes II. The reviewer can sometimes find mistakes. In my experience, this doesn't happen as often as you might wish, so don't count on it.

The first items don't depend on the experience of the reviewer, so you can be flexible there. I like to pick an expert in the area if I'm a little uncertain of my approach in a critical area, or an novice in the area if I want to teach something with my changes, or sometimes both.

As deadlines approach, risk management becomes more important, so getting more code reviewers is a good idea.

According to Promiscuous Pairing and Beginner's Mind, getting people out of their comfort zone causes them to learn very quickly. That suggests that either the coder or the reviewer should be unfamiliar with the code being changes.

Answer 9

The best code reviews in our corporate environment seem to have the following in common:

1. Attendance is limited to developer, a Sr. or lead from his team, the project manager and the technical lead for the larger system (we don't use/misuse the term architect here) and a Biz Analyst representing the stakeholders.
2. It happens before the UAT cycle starts.
3. The tech lead does a one-on-one with the developer early and often so the code review doesn't dwell on how to use findbugs and what version of java we are using.
4. Both the biz goals and the technical reqs were given to the developer early on so the review can focus on what was done right and wrong in meeting those goals
5. The review focuses on making the biz user feel confidant that the software is going to work well, the tech lead confident that they are not accruing unreasonable technical debt.
6. The developer walks away with ideas on how to make thing better in the future.