Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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: code review: Is it subjective or objective(quantifiable)?, but I'm looking for how to comment on it.

share|improve this question
2  
besides throwing names of FishEye and Crucible (my favorite tools btw) I see nothing programming specific here. One could get plenty advice on stuff like that by searching web for something like how to provide constructive feedback –  gnat Mar 29 '12 at 14:25
1  
    
@caleb, I disagree, this is more about how to phrase the comments than that other thread was. –  HLGEM Mar 29 '12 at 17:56
1  
@HLGEM I'd say that's exactly what the suggested dupe is about: "How can I tactfully suggest...". In general, focus on solving problems that exist in the code under review, not on style or your own personal preference. Explain how your suggestion makes the code better. –  Caleb Mar 29 '12 at 18:06
    
@stinkycheeseman just let the other people know that do it your way is better. and people in your team will learn something through the process. –  upton Mar 30 '12 at 8:35
add comment

5 Answers

up vote 7 down vote accepted

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.

share|improve this answer
    
Re paragraph #3: My experience is that merely explaining a better technique is rarely good enough (unless it's obvious). You often have to re-write the code before they will fully appreciate the benefits and become a believer. In a comment-only review system, this is hard to do. You might need to finish your comment with a "Come see me and we'll discuss it." to make it worthwhile. –  mcmcc Mar 29 '12 at 17:16
    
@mcmcc, that's a fair point, or you might refer them to some other place in the code where a similar techinique is used. I usually just use the comments to trigger an actual discussion afterwards unless they are all trivial. –  HLGEM Mar 29 '12 at 17:55
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
+1 just for the snorgatz factor (well I liked the rest of the answer too) –  HLGEM Mar 30 '12 at 14:06
add comment

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.
share|improve this answer
add comment

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.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.