What feedback is or is not appropriate for a code review? [duplicate]

Question

I am interested in hearing some guidelines as to what feedback is or is not appropriate for a code review. My team has a very unstructured review process and I am trying to suggest some ways to improve it. I am no expert so I hope to hear from you experts out there!

1. Should code review comments be limited to actual problems with the code?
2. Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?
3. What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better? How do I draw the line between contributing ideas but not sounding like a know-it-all?
4. In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?
5. Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

Please, if you could, provide reasoning along with any answers. It will help this junior developer be able to really understand the best way to approach a code review.

Answer 1

A code review is limited to reviewing the code being changed, and anything directly related to that. For the purposes of the code review, design or requirements are not appropriate for discussion. Ideally you should have settled on the design before code has been written, so if you find such issues during a code review, there was a failure in communication (someone misunderstood the requirements).

Don't get me wrong; it is very important to get the design and requirements right, no matter when you discover problems with them, but if you discover them during a code review, suggestions to do it a different way are not part of the actual review!

Should code review comments be limited to actual problems with the code?

Yes

Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

Unless they are related to the code being modified, no. For example, "I see you are changing these lines like so, it would be great to also change these other lines too" - is appropriate.

What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better? How do I draw the line between contributing ideas but not sounding like a know-it-all?

Not appropriate

In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

It's whoever has final say on what goes into the code base. This is unrelated to the code review process.

Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

Code reviews should be streamlined so that they never block a developer's process. Here are some issues to watch out for:

• Make sure code gets reviewed promptly; this means the person reviewing the code shouldn't be too busy
• The reviewer must be pretty knowledgeable about the code being reviewed
• Ensure that people never have to redo work after issues are discovered during a code review. That is, have clear specifications, or communicate between the coder and reviewer about what the proposed solution will be, before code has been written.
• Foster a culture of open reviews - let anyone be able to easily review other people's code, because code reviews are a great way to share knowledge between developers.
• Point towards the organisation's coding standards whenever differences in opinion arise. Ideally the standard is the final say, and if any conflicts arise because they are not covered by the standard, update the standard.

Answer 2

Should code review comments be limited to actual problems with the code?

Yes. The purpose of a review is to find where the code will not meet its specification (the design and by extension the requirements). If you find something odd/missing with either the design or the requirements, the author of the code is usually not in a position do do anything about it (at least not without incurring a significant delay). If you find such issues during a code review, you should enter a bug report for it and continue the review on the assumption that the requirements/design are actually correct.

Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

In a code review, you should only critique what you are being asked to look at. The only thing you should bring up outside that changeset are places where the author missed a required change or places where a change from the author should be applied as well to keep the overall consistency of the code at a high level. Any other issue you find during the review, you should enter into the bug tracker if it is important enough to warrant fixing. I would not bother too much with typos in comments.

What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better? How do I draw the line between contributing ideas but not sounding like a know-it-all?

Ideally, such things should have come up during the design (review). After that it is a balancing act and I would bring them up only if your solution provides a clear and measurable improvement in ways that are relevant to the success of the project. The balance here is between the advantages of your solution versus the costs of implementing it.

In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

The final say on such issues should lie with the coding standard. If the coding standard does not rule on the issue, it is best to accept the view of the person who knows the code base best, because that is likely to keep the most consistency in the code base.

Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

As a reviewer:

• Try to give the review priority over other tasks

As an author:

• Select your reviewers carefully. They should not already be overburdened with work and they should know the subject matter and code base already.
• While waiting for the review, see if you can start on the next task without messing up the code that is currently under review.

Answer 3

As a junior developer, you are not in a good position to make a difference about the effectiveness of your team's code reviews. That's really for the leader(s) in the team.

My two cents is that it is the outcome that matters about the code review, not the process. Reviews that don't result in substantially better design and code are a waste of time. Structure can help. But it also can hinder if it inhibits the team from addressing the real problems.

So ...

1) Should code review comments be limited to actual problems with the code?

It is hard to draw a line. It also depends on whether this just a code review, or also mentoring.

2) Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

It depends. If there is a need to improve the pre-existing code, and the possibility of doing it in the context of the work being done, it could be useful to talk about.

3) What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better?

If 1) the alternative way would give a significantly better outcome, 2) it would be practical to implement it now, and 3) the person who would do that work is likely to be receptive, then it could be appropriate.

How do I draw the line between contributing ideas but not sounding like a know-it-all?

Use your common sense, and be sensitive to other peoples' reactions.

4) In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

That is for the team / management to decide.

5) Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

If code reviews are obstructing progress, you need to discuss this with your project leader. The team leader carries the can for project delays and quality issues.