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

Re:Ask HN: What tone to use in code review suggestions? by simonbarker87 on Monday June 27, 2022 @02:52PM (#31898991)

I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders.

Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone.

Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if they have time or want to implement the change then they can. Shoulds can only be ignored with a reason and the reviewer has to agree or the change should be made. Musts have to be implemented and should be used rarely. The only person who can over rule a must is a department head, lead or CTO type person.

This massively reduces friction in the CR process.

Article here: https://allthecode.co/blog/post/how-to-get-better-at-code-re...

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by evsasse on Monday June 27, 2022 @03:27PM (#31899559)

Yeah, that is great.

Learned something similar on a previous job. Prefix it with "#must", "#should", "#could", "#would".

We've also extended it with some other things that you may want to comment, but are not necessarily actionable. "#wont", "#idea", "#tip", "#question", "#risk", are pretty much self-explanatory.

"#fun": an ugly workaround that could've worked; some funny situation that the change could cause; ...

"#domain-knowledge"/"#debrief": really nice if you are working with people that are more junior, or not that familiar with this codebase in particular. May help other reviewers understand better why the author made certain decisions.

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by darekkay on Monday June 27, 2022 @04:40PM (#31900486)

We do something very similar:

* NICE: Optional change. PR is approved.

* SHOULD: Highly suggested change. PR is neither approved nor rejected.

* MUST: Must be fixed. PR is rejected.

This also helps the PR author to know which of my comments have to be addressed to resolve my rejection.

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by another-dave on Tuesday June 28, 2022 @08:25AM (#31906768)

I saw another person talk about Red/Amber/Green — where Red is "fix this, it's wrong"; Amber is "here's a consideration, do with this what you will" and Green is "I don't know but I want to ask questions to learn".

So "Green: Why did you do it this way?" immediately comes across differently to "Red: Why did you do it this way?"

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by oldjavacoder on Monday June 27, 2022 @08:56PM (#31902867)

The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by jpace121 on Monday June 27, 2022 @09:15PM (#31902978)

Can you explain why? I’ve definitely seen must used in non military contexts. It also seems really strict to limit musts to things specifically laid out in requirements, unless I’m just used to dealing with really loose requirements. My gut is that you’d run into two categories of shoulds, one which is a much bigger deal than the other.

[ Parent | on HN ]

Re:Ask HN: What tone to use in code review suggestions? by sethammons on Tuesday June 28, 2022 @09:01AM (#31907128)

And protocol specifications

[ Parent | on HN ]