How can I be a nice reviewer?

Question

From Remove Nth Node from End of Linked List:

Bug: n < 0 leads to you removing the last node.

Your solution should have comments: I have NO idea how it works. And that's after reading it a couple times. If this was production code, I'd have wrapped it with unit-tests, thrown the implementation away and rewrote it.

How do I review honestly, without being overly harsh? I have a tendency to post an answer like that, and it's only after rolfl mentions it (@Pimgd - not a bit harsh, is it?) that I notice.

Answer 1

Empathy...

I am not necessarily a good example of a good reviewer, I have been the 'victim' of many reviews, and I have given a lot too. I am often tactless, and pedantic. I blame that on the fact that I am South African born.... but I have since moved to Canada, and culture here is much more sensitive to peoples' feelings. I'm sorry.

I have had to train myself to approach people differently, to be more empathetic about things, and to approach criticism in a different way. I don't always get it right, but when I do, it makes a big difference.

So, how to be a nice reviewer:

1. assess the 'competence' of the person writing the code. You should be able to tell a novice, from an intermediate, and experienced coder.
2. identify those key areas where a person should focus in order to elevate themselves to the next step, or two. Give advice that can be realistically applied.
3. praise the things done well - reinforce the good things.
4. always remember that the person who wrote the code is emotionally attached to it, and they were courageous enough to post it here for criticism. Respect that.
5. provide positive actions to take as well as (or instead of) negative habits to break.

---

So, to review your review....

Your initial assessment for the bug is valid, but did you notice that the OP includes the following check:

    if(head == null || n == 0)
        return head;

That's a good thing, they have pre-validated their data, which is good practice. They should extend that validation to include the negative-n edge-case. It is a small oversight, really.

How about saying it this way instead:

It is good that you included pre-validation checks to ensure the state of the system and arguments, but you have an edge-case you do not cover, which is that n may also be negative.

About the comments (I see now that the post has been edited.... hmmm rollback needed... but, in a bit...). how about if you said:

I struggled to understand the algorithm you are using. Your code has what look like meaningful names, but sometimes you need to include comments that describe the algorithm you are implementing. Good comments describe why you are doing things, not what you are doing. You have no comments, so it is hard to understand why you do what you do.

Answer 2

Be impersonal

The goal of code review is to improve the quality of the code (and to produce shared knowledge that will contribute to the quality of code written in the future), not to evaluate the worth of the programmer.

A reviewer can make it less likely that someone will take the review personally by directing criticism at the code, not at the person. Instead of writing:

n < 0 leads to you removing the last node.

a reviewer could write something like:

passing n < 0 causes the function to remove the last node.

which is more accurate, and might feel less harsh.

Answer 3

This ought to be an interesting discussion. Let me pitch in a few things. Here is what I picked up from your answer, at a glance.

1. It is honest, which is good.
2. It points out a bug, which is also good.
3. It doesn't really offer else much in the scope of improving the code, aside from pointing out that it is unclear what the code does.

I've read quite a bit about people skills, and here is some precious summary advice from Dale Carnegie's famous book:

Fundamental Techniques in Handling People

1. Don't criticize, condemn, or complain.

2. Give honest and sincere appreciation.

3. Arouse in the other person an eager want.

This goes against all 3:

If this was production code, I'd have wrapped it with unit-tests, thrown the implementation away and rewrote it.

See, I was about to do the same thing myself. It's easy to slip. I think asking yourself "Would this answer make me want to come back and ask another CR question?" before you post it is a good starting point.

Answer 4

How Would the Answer Make You Feel If You Received It?
There are already a lot of good answers here already.

I usually write my answers on my computer and then copy them to the webpage. This allows me to read my answer before I post it. When I read it, I try to make myself see how I would feel if I received this answer. Try to read your answer before you hit the "Post Your Answer" button.

Many of us have gone through peer reviews (design as well as code). We all know how it feels when someone says "Your code sucks". Use that feeling to tone down the answer.