Hacker News Reader

Comment by herge | Hacker News Reader

I have the similar problem with code reviews. It is really hard to not sound harsh when giving a code review, especially in ones from junior developers where a whole laundry list of fixes comes out.

View on HN

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).

Does being stern actually produce better results?

Yes, much better. It establishes that we are a "clean code" shop, and that sloppiness isn't tolerated.

This is important when dealing with junior contractors.

How about...

"Can you think of names for these tests that would be more helpful when we need to debug why they're failing?"

"Check out the styleguide for indentation like this; consistent code is easier to maintain"

"Here's a place where we did something similar. See how this logic goes over here instead? That helps us keep x, y, modular"

Phrasing things a little differently can make feedback feel like a learning opportunity instead of a criticism.

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.

Also, "why not X?". It puts the assumption up front that the programmer has already thought it through, which is often the case.

When it comes to code reviews; judge the code, not the developer. It's ok to be harsh to the code, but always make it a learning experience that people can take something away from -- and as a lead or manager, it's important to give your engineers the understanding that code reviews can be harsh, but it's meant to help improve everyone on the team, and to learn from anything someone says.

My only advice is to not worry too much about it. One of the first things a new dev needs to learn is how to separate critique of the wok from critique of them as an individual.

IMHO, code reviews should be clear and concise. They aren't a place to go out of your way to soften blows. I expect the same when my code is reviewed.

Edit:

It occurs to me the parent may have been referring to an informal review or one with the intent of mentoring. Asking questions, like the sibling mentioned is a great way. My comment is geared more towards a formal review.

I'm currently working as a junior dev. Not by title, but by experience. Honestly, I appreciate getting a laundry list of fixes over none. I know either way I can be doing better. As for sounding harsh, I feel it's a very subjective thing.

There's a developer where I work who has an amazing ability to make you feel good about code reviews --- I come out of one feeling pumped and enthusiastic even though they've just shredded my code apart and now I'm going to have to do all that work again.

Conversely, I've come out of reviews from other people, where they've been fundamentally happy with my code, feeling depressed and miserable.

Communication skills are vitally important. I've been trying to learn from the first reviewer; next time they visit I should actually grab them for coffee and talk about it...

I work with a couple of particularly arrogant junior devs. In code reviews I will tell them things like "the way you're doing this works, but it's better practice to do ..." and they just shrug it off, saying they don't really want to change it, they just want their code merged. Drives me crazy. So junior devs, and really ALL devs, please be humble about your abilities or you will be terrible to work with.

It sounds like your org is lacking in some basic leadership. If issues are brought up in the code it either needs to be corrected or justified with the team.

It is hard, particularly when management has put someone in a position they don't have the training for. In a junior developer that's understandable, but still painful when you have deadlines. Luckily, most developers are willing to learn and pretty good at seeing the patterns[1] of things done by the team.

To me, the biggest thing is don't lie to save feelings. You need to tell the truth or they will feel a whole lot worse later. Loosen yourself up. Don't go into that ridged pose. Talk evenly, and if you can manage it act in a jovial mood as you walk in. For the love of all you hold holy, know what they did code-wise, and take some damn notes before hand. Winging it will be the death of their trust. Teachers you don't trust aren't going to teach you anything.

I had a job where I was the C code reviewer, but was not allowed to program in C[2]. One developer, a fairly senior one, wrote some of the worst code I've ever seen. My code review was basically C 101 and it really was a tough chore to keep myself following good body language and words while still indicating the code needed a lot of changes to be acceptable. I really do blame the employer for setting the developer up to fail (no C training, just read a book and wing it). I did it ok, but I really wish I had a mentor of some sort in the room to give me a critique of my critique.

1) I really don't mean design patterns, more like the general pattern of code with that project / workplace.

2) Well, obviously I couldn't write C code since there would be no one to code review mine. Not obvious? Wasn't to me either to tell the truth. I got to spend my days programming in a reporting language and AWK. I only lasted 9 months (my 3 seasons in heck).

This is a matter of being friendly with the team on a daily basis. If you are, then you can change a sentence that could sound harsh without the context into friendly conversation which may teach both of you something new. If you have found anything bigger then one line, sit together, have a laugh or joke (friendly one), chat about the issue, think (both) about improvement. Not so difficult, really.

I find it's a good idea to throw in some positive comments as well along with indicating the severity of different comments (e.g. must fix, nice to have).

A far worse situation though is when a mid-level or senior developer is defensive about code reviews and won't cooperate in the code review process.

This is one of the main reasons I prefer pairing w/junior developers over code reviews. Giving lots of small pieces of feedback feels a lot less harsh than a monolithic dump of everything that's wrong.

Reply on news.ycombinator.com