What's the best way to comment in a code review?

Question

My team just started using crucible/fisheye for initiating code reviews whenever one of us checks something in. There are only 3 of us, and we're each encouraged to review the code and leave comments where we see fit.

My question is, how do I best leave a comment on a line of code I see a problem with? I want to get my point across without seeming abrasive.

I don't want to seem like I'm on a high horse and say "I've been doing it this way... and I also don't want to seem like I'm trying to be authoritative and say something like "This should be done this way..." but I still need to get the point across that what they're doing is not very good.

To Clarify: This is a really good resource for what I should be looking to comment on: Is a code review subjective or objective (quantifiable)?, but I'm looking for how to comment on it.

Answer 1

Well I tend to make comments in several general areas and each type might be handled differently.

Required changes. These are the kinds of changes where I point out that the code doesn't meet the functional requirements or doesn't work and must be fixed before being pushed to production. I tend to be very straightforward in these comments. The requirements says..., this does not do that. Or this will fail if the value sent in is null (especially when you know that case will occur based on the data that will get sent in).

Then there are the "this works but here is a better way to accomplish that" comments. You have to be more gentle in these and do more of a sales pitch. I might say that I would do this instead because it is likely to be better performing (I generally review SQL code where performance is very important). I might add some details about why it is a better choice just like I would do in answering a question on Stack Overflow. I might point out that it isn't required to change this for this particular code, but to consider the change in future coding. Basically with these types of comments I am educating people with less experience on what might work better.

Then there are the "this works but we do things this way" comments. These will probably also be required changes. These would include comments about company standards or the architecture we expect them to use. I would reference the standard or architecture document and tell them to fix to the standard. The comment would be straightforward but neutral, it is missing thus and so or the variable names don't conform to our naming standard or simliar things. For instance, our architecture for SSIS packages requires the package to use our metadata database to store particular information about the package and requires particular logging. The package would work without these things but they are required for company reasons (we need to report of the success rate of imports for instance or analyze the types of errors we receive.)

The one thing you don't want to do in code review comments is personally attack someone. It can also help if you find something they did well and point out that was good. Sometimes I learn something new from a code review and if I did I tell the person that.

Answer 2

If the code follows your coding standards, but you'd do it a different way you have to ask yourself if what they did is wrong.

If it's not...it's just not how you would have done it and you just can't leave it, try just asking 'Why did you do it this way instead of that way?' Then you are getting them to qualify why they did it the way they did without saying 'I would have done it this way and you should too...'

You might also learn something in the process.

Answer 3

I want to get my point across without seeming abrasive.

Don't confuse terseness with being abrasive. When something is a problem, document it in a way that whoever is going to fix it can understand. Stick to the facts and don't write an essay. To wit:

• This will cause the frobnitz to malfunction when fooble is within 5 grimbles of the snorgatz factor.

• The established convention for doing this is to call fazzatz() with a freshly-initialized Squidge. Make this into a method so it always happens the same way and isn't duplicated.

  I also don't want to seem like I'm trying to be authoritative and say something like "This should be done this way..." but I still need to get the point across that what they're doing is not very good.

The purpose of reviewing code is to put a second, usually-more-experienced, pair of eyes on it to ferret out problems. If you're in the position of passing judgement on others' work and there's a valid reason to say something isn't good, you'd be neglecting your responsibility as a reviewer if you didn't.

There will be disagreements, and those are opportunities for the reviewer and reviewee to defend their positions. If you're otherwise peers and you reach an impasse, find someone senior to break the tie.

Answer 4

It depends on what sort of problem has been noticed

• missing copywrite notice - a common and boring issue just a brief comment stating the issue and move on
• places where I might do it differently - usually tend to ask questions here rather than make statements, sometimes the answers will justify the original solution other times not and then I can address those more explictly
• places where there is a clear defect e.g. Equals override that can stackoverflow - reach for the red pen - mark it as a defect and be very explicit as why it is broken - also check other similar areas to check there has not been a systematic problem.

Answer 5

From my experience:

1. Always have the author of the code with you while reviewing his code. Preferably the code is projected on the whiteboard and both of you can clearly see the code very well.

2. Have a friendly conversation. Appreciate the good part of the coding. Tell him that "this is the best I have seen" if you see some good parts in the code.

3. Ask him to review your code and accept and agree to the valid points and correct him. Give respect for his comments in your code and he will automatically give respect to your code review comments.
4. Deal with at the developer level unless it is very important or more time is needed to correct the code review problems. Don't escalate to managers for simple if condition missing issues.
5. Look with the perspective of "Learning from other code" instead of pointing mistakes in the code.
6. During the code review sessions, quote the past mistakes that you have made and how code reviews helped you and avoided big production issues because another set of eyes helped you.
7. Be humble. More appreciation and less comments to him :) You will learn a lot during code review and he will also gladly accept your comments.