How to code review without offending other developers [duplicate]

Question

I work on a team that does frequent code reviews. But it seems like more of a formality than anything.

No one really points out problems in the code for fear of offending other developers. The few times I've tried to ask for changes were met with very defensive and reluctant attitudes.

This is of course not good. Not only are we spending the time to code review, but we're getting literally zero value from it. Is this an issue that needs to be addressed by individual developers, or are there techniques for suggesting changes without stepping on other people's toes?

Answer 1

This seems to be a pretty common prevailing attitude among some developers. Everyone seems to feel that a code review is some challenge to their work, and that makes no sense to me. A code review is a quality assurance mechanism that has the added bonus of education to go along with it. We implement code reviews extensively where I work, and I've fostered within my own team the attitude that the code reviews are a collaboration mechanism more than a quality process.

The only way to begin coding as a team is to see each other's work and to question it. That's how best practices are formed. Dialog is the key. I've sent code back to developers for silly reasons such as formatting, agreed upon best practices, and spelling. I code review with a very fine edge, and I expect my own code to withstand the same scrutiny. I've used tactics such as checking in code that would not pass my own code review for the sole purpose of getting a junior developer to challenge me. Tell me it doesn't meet the requirements. Stand up and have an opinion.

Some guidelines I think everyone should really have when engaged in a code reviewing environment:

1. Get over yourself. You're not perfect, and just because a junior developer managed to see you do something silly in a for loop isn't the end of the world. If you're so great, invite criticism and prove it. Expect code reviews to reveal new ideas on how to do things, new habits, and, most of all, constructive dialog.
2. Keep an open mind when reviewing someone else's code. Be receptive to their ideas, and make sure that changes you suggest are suggested for a reason. It is assumed that the checked-in code builds, but that doesn't mean that it follows best practices. Make sure that anything you bring up can be backed up with a cited reference. If you say it doesn't follow best practices then cite the standards document and section. If you say it isn't a "performant" method, then have a link to a document that shows why and possibly provides metrics.
3. Make suggestions useful, and explain why you're suggesting something. You will occasionally find a problem with code that is self explanatory, but most of the time this person has coded something based on habits. Explain why this habit should be altered and the value of altering it (unless you're explaining it for the 5th time, in which you have a personnel problem).
4. When your code is reviewed, consider everything the reviewer is suggesting objectively. If you're pushing back, ask yourself honestly if you're just being defensive or if you really believe you have a case. If you have a case, continue the dialog. Don't get argumentative, bring ammunition such as facts and metrics.
5. Whether you're a reviewer or a reviewee, use code reviews as an opportunity to educate. Whether it's educating yourself or the person you're reviewing, if there is a discrepancy then there is a chance to learn somewhere. Make good use of it.
6. Ask questions and be ready to have your questions answered truthfully. I recently made a statement that was "pseudo-true". It wasn't wrong, but it definitely wasn't right. A junior developer challenged me on it, and I disagreed. My response was "I haven't seen that behavior, but if you can find me a document on it I would love to read it.". I spent about an hour that afternoon reading the document he sent me, and now I have a much better response (re: educated) when confronted with a similar situation.

Given the education slant that I put code reviews in, I often make my responses about questions. If I find something blatantly wonky, I will instruct the developer to correct it. Otherwise I will ask the questions. "Why did you use method A to achieve goal B?" "What gain does declaring a variable have in the instance of its usage in method Z?" "I see you have copied/pasted some code, did you consider refactoring? You didn't refactor, what was the reasoning behind that choice?" The code doesn't progress until the reviewer approves it, and the reviewer won't approve it until the questions are answered. When framed in an inquisitive way that indicates you don't understand the developers reasoning it becomes less confrontational and takes on more of an instructional vibe.

Answer 2

In order to help people take a code review less personally, try to target criticisms and suggestions to the code itself and not its author. For example,

The function "getFoo" should do xyz because....

is better than

You should do xyz in function "getFoo" because...

On the other hand, if you want to express a positive opinion you can praise the author. For example,

I like how you used abc algorithm because ...

Answer 3

First, the team should agree on the goal of the code review (you should not take such agreement for granted, as code review can be used for a variety of things, and different organisations do it for different reasons. For this answer, I will assume that the goal is to improve a developer's future work.

To improve, the developer needs to know

1. what was good? -> I should continue doing that
2. what was bad? -> I should change that
3. and why? -> So I can assess the severity of the issue, as well as recognize similar issues in the future

What action is this about?

Feedback should be about a behavior, not a person. Therefore, it should identify (preferably with examples) the behavior the feedback is about. So don't say:

You're lazy.

because that isn't about behaviour, but (perceived) personality. Also, it's too vague: Is this about the long lunch break? Slow typing? Not answering email in a timely manner? Too few implemented features? Instead, you should say:

Last week, you were assigned bug #1059. You have marked this defect resolved, but the bug still exists, and can be reproduced with the instructions provided in the trouble ticket.

And Why?

Next, you should describe why that action or code was good or bad. The reason given should be objective, and of sufficient importance to justify a change. For instance:

AClass.amethod() is invoked by concurrent threads, but not thread safe. Data corruption is likely.

or

This method name is misleading. It sounds as if we're merely calulating the pay grade, but actually also pays the employee. Somebody not familiar with the implementation is likely to misuse the method, causing accidental salary payments ...

Of course, sometimes you don't have an ironclad argument why the code is bad (or good). This means you don't know you're right. Therefore it is prudent to be inquisitive, rather than presumptious:

This looks awfully complex. Why don't you just invoke AClass.amethod() instead?

or

This appears to be nearly identical to AClass.amethod(). Why can we not reuse that method?

You can also give positive feedback, of course:

That's a great approach to coordinate the worker threads! I usually do X instead, and I always wondered how I could make it more robust. Now I know :-)

Please don't do this

To illustrate the importance of the above, let's consider a real world counter example taken verbatim from a formal code review:

The code leaves a poorly thought out impression.

And that's all the reviewer wrote about that finding.

That is terrible feedback on so many levels:

1. The feedback implies something unflattering about the author (namely, that he thinks poorly)
2. The feedback is about an impression. That is not objective.
3. The feedback does not identify any part of the code that needs to be changed,
4. nor give any reason why it should be.

The recipient of such "feedback" can not know what he needs to change, nor why, and thus not learn anything. In fact, he can not even know that the feedback is fair, and, given the unflattering unsubstantiated conclusion, is far more likely to see this as a personal attack by somebody who wants to elevate himself by pushing his peers down. Shouting is likely to ensue.

Answer 4

A code review shouldn't just be a fail/pass with only fail returning detailed feedback. This tends to focus on the negative and turns people off from exposing themselves to the criticism.

Reviewers might also give a more negative response to get more limelight in the eyes of the manager in terms of "look I'm doing my job and criticizing everything that comes across my desk"

If you add a comment field that is returned to the submitter fail or pass then he'll feel more positive about his work and the reviewer can still give feedback about what could still be better even if the code passes the review. This also would allow the reviewer to do a limited refactor of the code (formatting, renaming variables) without needing to bother the developer for the trivial changes besides notifying him of them.

Answer 5

They say that "code should be owned by the team", so a review is not so much a personal attack on an individual coder, but a quality step to ensure things are understandable by the rest of the team. I'd say most places I've worked, this is understood and accepted.

However, I did interview a guy who was so against the idea of reviewing other people's code that we rejected him as a candidate! (that is - not of having his code reviewed, but doing reviews... it was weird).

That made me think a little about the problem with how some people approach reviews. I'd say a better way of doing code reviews then is to remember the first line on my answer, and build on that. To do this is quite simple: a code review stops being the perceived "someone checks out my code and tells me what I've done wrong" and becomes an opportunity for the coder to tell someone else what their code does, how it does it, and what changes they've made.

In this case, the coder will end up reviewing their own code, with an extra pair of eyes to help spot the bit they might miss. It's also a chance for the coder to explain why he did things a certain way. Such an approach is just as effective (if not more so as you have a good exchange of communication) and considerably more friendly. I don't think it would take more time considering the review should be quicker as the original coder can explain the changes directly. People should also understand the need for such a review as they are "handing over" the knowledge to another team member who might be required to maintain the code in the future.

You can enhance this by having the 'reviewer' come prepared with the original requirements for the change, and tick them off as their solution is detailed, so it would feel even less like a code review.