Balancing the critique with some positive feedback - parts you liked, appreciation for the features shipped, etc. - is one idea I've seen floated around, for a particularly touchy coworker.

Also, generally managing tone - "Looks good! Don't forget to add the doc comments on functions X Y and Z" vs "Why are X Y and Z missing doc comments?". Both have the same fundamental information (X Y and Z could use doc comments), but:

With the former, you're acknowledging progress, a "good" first draft, and pointing out what remains to be done in an ego friendly way ("I know you're good enough to make this even better") that doesn't demand a response.

Meanwhile, the latter demands a response, all of them negative. Either "it doesn't need them", or "I forgot them (because I suck)", or silently ignoring the question. None of these are ego friendly.

.

If the code just plain sucks, however... I'm not really sure what to do.

And if the coworker gets defensive even with positively slanted feedback... maybe there needs to be a conversation with them, to try and shift their mindset. Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.

Maybe get them in your shoes - helping critique someone else's code, and help explain that they're not being an asshole when they point out edge cases and ugly code, but that they're being an asshole to future them when they have to go back and add a feature, fix a bug, or handle a corner case they'd missed.

Thanks for the feedback. We are a small team (3 embedded sw developers) and I am more or less the guy with actually production releases under my belt. I feel my approach has been very measured and very polite to the point where I feel like i am walking on "egg shells" when doing the code reviews.

Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.

within a few days we're going to be trying exactly this in hopes to show how code reviews are not about attacking and instead about helping.

It's a problem for small teams and people with little experience with code reviews. The 'walking on egg shells' thing happens ALL the time.

I've never seen this get solved overnight. Your management really needs to sit down and explain their expectations around code reviews - that fixing things based on co-worker suggestions is a positive thing, that having negative comments won't affect their performance reviews, etc.

I also find that with people like that you really do need to let more things go. If it's not a critical bug or an actual problem with the code then let it slide. Some people just cannot take criticism (even constructive) and the bad blood you'll get by persisting with it will not help you.

I also tend to find these developers are not necessarily the strongest devs and often have other personality problems. This likely indicates that your company needs to focus a lot more on the hiring and culture.

Try taking power away from the reviewer to disarm the defense reaction. In Ed Catmul's Creativity, Inc.

(http://www.amazon.com/gp/aw/d/0812993012/ref=mp_s_a_1_1?qid=...)

he describes the Pixar "brain trust" which reviews early progress on movies with the director. They found directors would get defensive if they felt someone more powerful was telling them to make specific changes. Once they set some rules like "the director decides what, if anything gets changed" and "talk about what's not working but don't solve the problem for the director" things improved. Now instead of film veterans telling you your movie (code) stinks it becomes about opportunities to make things awesome.

If you know that your colleague is sensitive to what appears to be the overwhelmingly “negative” comments, make sure that there are positive statements. I had a code review recently for one of my guys that had a lot of comments that were style and logic problems.

When I finished the comments on the code, I made sure I added a comment to the top of the pull request (we use BitBucket) indicating that it was clear he understood the point of the code and had written a good first pass at it, but there were a few things that needed to be dealt with for idiomatic code.

+1 I'm currently dealing with this. The individual doesn't offer feedback on my code when under review, so it appears like a personal attack.

It is important that code reviews are public enough that people see other people review each other's code - a system where the only code reviews you see are the ones you do and the ones you get leads to poor expectations of what they are for.

Having at least two very accomplished and (culturally and organisationally senior) people routinely "model" reasonable review behaviour can be stunningly effective.

Also this is an area where frequent team conversations about what good code is outside a review situation helps to build a certain culture. It helps step away from nitpicking and arguing.

Go have coffee (or whatever) with your colleague and have a 1-to-1 conversation with them saying you want their feedback on code. You want their expertise to make you a better software developer. Even if you are better at this than they are, you can learn something from the sharing.

The team’s lead should make sure that the individual in question knows that software development is a cooperative process and that code review helps build the team.

This. Although I'm less civil about it - I'll start giving people shit for rubber stamping my code reviews.

"Why are you letting me ship this terrible code I wrote? Do you want us to end up in a death spiral of technical debt and crunch? Don't be an asshole, critique my shit! We all need a second set of eyes - I'm no exception!"

...okay, I'm maybe a little more civil than that. But I'm not above leaving some mistakes unfixed to call them out on.

Ha! I'm like that with my QA people, begging them to come after me with a blunt weapon.

We do the bulk of our reviews at the pull request stage, as line comments on commits (we use Github). These are visible to all team members. This form allows developers to take the time to phrase the feedback diplomatically.

I've also set the tone early on in my own comments (I run the development division) and I've encouraged the more experienced and talented members of the team to review my own pull requests. When they spot a problem in my code, I always make it a point to praise them (if, of course, the problem is valid). This seems to have worked.

Try with design sessions upfront, i.e. discuss how the problem should be solved in terms of patterns and architecture. Once that is agreed, with 2-3 people, then the code review becomes simply a matter of style and there you can only invite for consistency with the rest of the code base. Often the problem is in the tools, face to face conversations, pair programming, human interaction help with that. You can also have your colleague help with a task of yours and show that you appreciate his input.

+1 This is something I've been meaning to introduce. We're slowly moving from the idea of hacking away and making things work to the idea of making production ready code and this mentality is not getting through to everyone

Try a compliment sandwich... Sandwich the criticism between two positive statements.

e.g. I like the way you did this, but I think this would work better but you're on the right track.

The feedback sandwich is something to be avoided. People are generally predisposed to either hear positive or negative feedback, but not both. When you give sandwiched feedback, those that easily hear positive feedback will miss the negative and those that easily hear negative feedback will miss the positive. Almost no one will hear both.

Or so says the management training courses I've recently taken. Supposedly there's studies to back this up, too, though I've never read them myself.

Have you considered going out to lunch with them or grabbing a coffee and bringing it up? If you're in an organization where you wouldn't be comfortable with that, maybe talk to your manager and ask them about it? Basically, we all have to toughen up a little bit when getting reviewed and it is for the good of the project/team. So getting someone who is uncomfortable past that is a win-win for everyone.

Sounds like he told you exactly how you should deal with it. Stop focusing solely on on the bad things and mistakes: make sure to mention "hey, neat idea" or "this is pretty clever!" on parts where, you know, it is exactly that. Diplomacy is a useful skill.

Sounds like he is too attached to his code - maybe make it abundantly clear that you are critiquing his code not him. If you have the ability to do so, try to have him fix bugs in other peoples code (that way it might feel less like his code).

I'm assuming you are performing asynchronous code reviews (like pull request comments). If you have the time, one of my teams did occasional "Code Inspections", where a team of 5 people would review a larger chunk of functionality together.

The key is that each person has an assigned responsibility, and that one of those is the "Reader". The reader presents the code for review, and the reader is NOT the author. The author is there, but I think it helps when someone else is presenting the code to reduce the feeling that the review is of the author.

Ask your manager to make the review a formal and required part of the process, for everyone. That way everybody is treated equally. If you go ask your colleague out of the blue why his code looks the way it does he might get defensive and wonder why you were suspicious to review his code behind his back.

I have, and it was more an attitude/cultural thing: the reviewee took any criticism very personally, and was hence super defensive and unlikely to change anything anyway.

Honestly, I think it's actually really, really hard not to react that way (to varying degrees) especially as most code-reviews basically come at the wrong time: you've already written the code, it works, it has glorious unit-tests -- who cares if it's a for vs while loop or whatever. It's actually even worse if there are serious design or maintainability issues, as that's even more code to re-write/"fix".

Personally, I really like having code-reviews -- it's better to get rid of serious problems if they're there. However, I've found a lot of reviews to be rather worthless "rubber-stamp" exercises -- especially if the dev team has just been told Thou Shalt Do Code Reviews Because Reasons! (I've also found the opposite, for sure: getting nice solid advice or missed cases; but these are invariably with other developers who want reviews no matter what management said).

...but all that said, I think so-called "code reviews" really work best if they're spread out over the coding (unless it's really small chunks at a time) -- a bit of pair-programming (or "hey, can you help me with ...") and design (or whatever you call "early on in the cycle") reviews make it a hell of a lot easier to take a different approach.

I think they used to call this "collaboration"?

Companies that just blast out a "new rule: everything gets code reviews!" memo without helping people understand the benefits, and how to do more "along-the-way" validation of design, refactorings, etcetc are always the very worst at having code reviews that are in any way effective. At such places, I find "code reviews" devolve into either useless "rubber stamp" affairs or something like what you describe. Or worse.

Also, if you have "Junior Developers" blasting out code for a couple days (or more!) with no adult supervision with a hope that some last-second "code review" procedure is going to equal amazing code, you're doing mentoring wrong. It's a lot nicer for everyone involved if there is someone sitting down with (or chatting or watching-the-commits-of or whatever works) the less experienced developers. These mentors should be providing hands-on, practical advice as they go. Some devs might need more hand-holding, some will need "a plan", maybe you need to sketch out classes or method-signatures for them, perhaps they need a bunch of half-day tasks written down, etc...Such mentoring should, IMO, be what your Senior Developers are spending a decent chunk of their not-coding time on.