Best way not to sound harsh is to ask questions. "What are your thoughts on ...?", "Is this really what you meant to do?", "Do you think there is a better way to handle...?". It puts the power and learning opportunity back to the other person and lets them feel the accomplishment of improving.
To the junior devs, when you screw up (and you will) own it and learn from it. Like public scandals, the cover up is almost always worse than original issue.
I find that tone works when the developer has clearly thought things through.
Other times, though, some developers need a very stern review. Things like: "Don't name your tests test1, test2, test3. Give them descriptive names," "Follow style," and "this does not belong in the dependency injector," and "don't screw with event publishing logic, filter this out in the event handler in the UI" are warranted when a developer isn't taking the time to think through his/her changes.
A lot of that can be softened with "This is the project's style. I don't agree with everything in the style guide, but consistency is important." You can say that, even if you agree wholeheartedly with the part you're calling out.
Never say "do it this way". Try to explain why "this way" is better so they learn. If you don't they'll just think you're saying "my way is better" and resent you for it.
Stern really is the last resort mainly because the things that people really argue about are often just opinions. No one is going to argue that test1, test2, etc... are good test names. It is certainly better to let someone realize that on their own with a question than sternly tell them they are bad.
The very rare times when I have to be stern is with consistency issues. Usually though, that's even softened with a "Yes this might be a better way to do it, but it is not consistent with how it has been done so far. We'll make some time in the future to change it everywhere, but for now stay consistent." Often times the biggest issue with juniors or even mid level people is they are only looking at their immediate piece, and not thinking about the larger application or system level picture.
Or... bring the team in to review some old code that no one has touched in a while and give everyone the opportunity to critique it. Set those critiques as the bar for everyone's code review. Then, when you review the JD, if he has made those mistakes, you don't have to be stern, you just have to remind him (or her).
I was subjected to this "Socratic" type of code review when I was younger and I didn't like it. I felt like I had to worry about what I thought the reviewer might be thinking as well as what was actually going wrong with the code.
I think just saying what you think but with a bit of humility and the attitude that you need to justify yourself is best. Especially since even the best seniors often get hung up on pointless crap during code reviews.
Yeah I feel like a lot of advice is describing what constructive critics do, rather than the attitudes that lead to it.
If someone is humble, they'll naturally tend to ask questions as described. What you've written isn't what they expect, but they assume you're not an idiot and there's a reason for it, so they ask why.
But if someone assumes they know better, the socratic method is likely to be just as condescending as just saying you're wrong.
> What you've written isn't what they expect, but they assume you're not an idiot and there's a reason for it, so they ask why.
This is exactly why I ask questions. It's important in a code review to understand the frame of mind of the code writer. I presume the person is not an idiot and did things for a reason or will respond with a doh! it was late/that was careless/thanks I'll fix it.
To the junior devs, when you screw up (and you will) own it and learn from it. Like public scandals, the cover up is almost always worse than original issue.