Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Ask HN: What tone to use in code review suggestions?

news.ycombinator.com

Post reply on HN
Print view View original

61–70 of 309 posts

Re: Ask HN: What tone to use in code review suggestions?

#61

Post by symby »

Perhaps, silence would be a good choice on this issue?

To my view, coding is as much art as science, and it is limitlessly fascinating to me how different people find different ways of expressing ideas and solutions.

Also, the code review process is fraught with opportunities for insult, misunderstanding and unfortunate power dynamics. It is inherently difficult regardless of the actual content being reviewed.

On the other hand, if there is a significant issue here "I am having trouble following your thinking here. Perhaps dividing this up into smaller functions would help?" Might be a good review comment?

The style wars are very tempting to engage in, but they virtually never drive greater productivity, real quality improvements, or positive team dynamics.

If there are real reasons that this wants to be broken out into a separate function (reusability for instance), then make that clear in your suggestion.

Re: Ask HN: What tone to use in code review suggestions?

#64

Post by pydry »

dyingkneepad wrote: post #56
pydry wrote: post #46

If it's negative feedback or something that feels controversial I dont put it on a PR at all. I try to do it in person or (remotely) send a private slack message. People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.

If this is on an open source community, it's bad advice IMHO. On open source communities, please make all feedback public. It is really important to make the history of the project public. There are many ways to write the feedback in a not-offensive way, many of the comments in this thread have good suggestions.

I was assuming a normal working environment.

OSS is a different kettle of fish. You dont have to worry about bruised egos nearly as much & the value of public feedback is higher.

Re: Ask HN: What tone to use in code review suggestions?

#65

Post by ratww »

livinglist wrote: post #4

“Won’t it be better if we did that instead of this?” “Do you think it would be better if we did that instead of this” “I think it would be even better if we do this instead of that” I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.

I agree. I tend to use "we" when for the negative stuff of anything that has to be decided by the team ("we changed the code here but it caused a regression", "should we refactor this?"), however I also like to use "you" for positive stuff and to give credit ("your change in b8e63dca also fixed this bug, livinglist").

Re: Ask HN: What tone to use in code review suggestions?

#66

Post by exac »

pclmulqdq wrote: post #39

What do you mean to say when you want to review code? Personally, I have roughly three distinct levels of things that I comment on in code reviews: 1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and…

I prefix my nitpicks with the same, but with the 3rd style.

Re: Ask HN: What tone to use in code review suggestions?

#67

Post by ysavir »

Remember to be problem oriented rather than solution oriented. If you just tell people "I think this should be the solution" or "you should change it to this solution", you aren't working with them, you're instructing them. And a key to teamwork, especially reviewing, is a cooperative spirit, not an instructional one.

If instead of saying what you would do, or what you think they should do, you express a concern about the code, the tone is completely different. Now it's just saying something like "do you think repeating this across files will be problematic?", which allows the other person to be the determiner. If they agree they may well do exactly what you would have suggested, and if they don't, it can start a discussion that helps bridge together how various team members code. It's win/win.

It also helps people understand what you aim for when writing code, which helps them keep in mind your needs in the future (and vice-versa for you to keep in mind their needs).

Re: Ask HN: What tone to use in code review suggestions?

#68

Post by XorNot »

pclmulqdq wrote: post #50
jeppester wrote: post #48

My personal favourite is: "Have you considered X" I also like to often prefix my suggestion with: "It might have missed something, but X", especially if it's a larger change.

I hate it when people say "have you considered X?" and mean "do X or I will block your change." It's all about communicating what you mean.

I agree with this so much.

IMO if it's not mandatory I don't think it should be said at all in a code review.

Re: Ask HN: What tone to use in code review suggestions?

#70

Post by mytailorisrich »

pydry wrote: post #46

If it's negative feedback or something that feels controversial I dont put it on a PR at all. I try to do it in person or (remotely) send a private slack message. People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.

Code reviews are primarily a way to detect bugs and to verify compliance with a set of rules.

Most comments are therefore expected to be 'negative' in some way.

Post reply on HN