Code Review Best Practices

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.

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.

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.

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.

I agree 100% with the "don't be a blocker." I personally look for about 80-90% yes and if it meets that criteria with no errors, I thumb it up. If I can't thumb it up, I try to put a "thumbs up with commented items fixed". I try to not block -- comment for bad issues, sometimes sigh but agree when it's a 10% disagreement and go forward. Life is a series of iterations. That 10% will be addressed in a future iteration.

And I never close another persons PR.