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

71–80 of 309 posts

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

#71

Post by mynameisvlad »

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…

> Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.

This. We do it on our team as well. Especially when it is smaller things like spelling, indentation, etc, I might have quite a few nits in one PR, so it makes it a little bit easier on the receiving end when you see a dozen comments but they're all marked as nit.

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

#73

Post by XorNot »

Use number #6. If it's not necessary, then why say it at all in a code review? Everyones got other things they'd like to do.

All the "should" and "considers" are tickets that can go on the backlog after a merge has delivered functionality.

Then YAGNI will probably turn out to apply.

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

#74

Post by harrisonjackson »

All of this should be hammered out in a set of guidelines around code review so that ego/feelings don't need to come into play. Have style guides, code review checklists, and best practices documented.

The actual words used should be determined by the goal of code review and the relationship between reviewers.

Anything that can possibly be automated by CI should be. Style guide and test coverage should automatically fail PRs before a human needs to add any "nit"

That means code review should be a teaching or question moment (between a senior and junior, newer and veteran) or a discussion between peers.

From a junior to a senior -> why did you extract this? why didn't you do it this way?

From a senior to a junior -> typically, I would extract this because X. Check our best practices doc for a longer explanation.

Between peers or senior/junior (maybe one is newer) -> it isn't in our coding best practices document, but normally we extract this because X

Between peers -> what do you think about extracting this because X? Should this be in the coding best practices document?

The last is important because ideally you're updating your coding guide so this same review doesn't need to be done 10 times. And, if you don't think it should be documented as a best practice then don't bring it up in a review.

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

#75

Post by ulisesrmzroche »

There's not a lot you can contribute unless you worked on the ticket. Notice how the only example you came up with was refactoring.

This was more important back in the day but now linters and formatters are mainstream.

The best time to code-review is at the beginning.

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

#76

Post by ratherbefuddled »

The emphasis should be on clarity. Overall you want to leave the requester in no doubt about what it will take to get approval because the most frustrating thing for both of you will be ping pong.

Tone is very hard to judge in written form particularly when there might be cultural and language divides, in my experience though it doesn't really matter what style you use provided that you also remember to comment on things that are good and do not need changing. This is routinely forgotten and more than balances any perception of negative tone the requester might inadvertently infer.

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

#77

Post by hluska »

There are three main categories of code reviews:

1.) Things are mainly bad.

2.) Things are mainly good.

3.) Somewhere in between.

Within those, everything I see and flag as worthy of discussion is either:

1.) Something that violates our rules/style.

2.) Something I have a strong opinion on.

3.) Something I don’t understand.

Depending on which main category the review falls into, I’ll deal with these differently. If things are mostly bad and I don’t understand something, I’ll ask a lot of questions about readability and naming conventions. If something is mostly good and I don’t understand something, I’ll ask “Hey, can you explain lines 71-104?”

In general, I try to keep things friendly and positive because I don’t want people to dread the experience. Life is too short to dread a big part of your job.

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

#78

Post by IncRnd »

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary.

None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question.

Consider:

* There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode.

* This portion duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go.

* While I don't have feedback yet on this function, it's too long for me to follow. It would be easier to read and for future maintenance if you refactor this part into a function.

* Since we're in closedown we can only take certain types of changes. If you refactor this into a separate function in the library, this change can be accepted.

* Or, I hate to be the process person, but the internal guidelines for this team call for all code to be structured the same way. Refactor this part into a separate function and I'll approve the PR.

There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."

How would you speak when sitting next to the person face-to-face? What tone do you want your boss to use when providing a performance review during a 1on1?

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

#80

Post by m3Lith »

vsareto wrote: post #29

There's basically two categories here: - you have an opinion on how things ought to be done and want a dialog - you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO). >* Should we extract this to a se…

What about clarifications? In a sense when you're not sure if this is good code, and would need to get more details. I sometimes just ask directly ("Why is this there?"/"What does this do?"), but other times I just don't bother and let others review.
Post reply on HN