6 ms·

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been mu

by ayberk 3y ago

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration. C++ readability was a much, much better experience. All the comments were about actually making the code better, eg, "use THIS_MACRO() instead of THAT_MACRO(), because go/...". I guess I think it's much more about the reviewer, and based on my anecdotal experience, the language :)

jgoodknight 3y ago
That was my experience getting Java readability too! Only 1 time in 20 did it feel like they were making the code better
tunesmith 3y ago
Getting reviews from multiple people that disagree on style is definitely an org problem that sucks to be in the middle of. However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an opportunity for how to improve this area of the code", that's better then acting like you made a mistake in failing to change it.
- xKingfisher 3y ago
  I try to avoid nits totally unrelated to the changes at hand, since on a subconscious level they may discourage people from even wanting to touch older/less loved files at all. The critical exception being avoiding issues due to path dependence. E.g while a change is "correct" is doing X poorly because of surrounding issue Y. So we should fix Y now instead of building atop it.
- eichin 3y ago
  Something I find helps with this in particular is only allowing style comments with citations to an actual style guide item. (I talk about domain-specific style guides as "crystallized arguments" - we agreed on this and wrote it down, not because it's necessarily right (though it probably is) but that we really wanted to stop wasting time arguing about these particular things.)
- compiler-guy 3y ago
  This can add cost and latency to the change author and distracts them from the project at hand. And because it costs the reviewer basically nothing to say, "While you are in there....", it slows the original author's work. Even worse, if there are enough of these minor changes, some other reviewer may take issue with their "small, focused changes" preference, and ask you to split it. This is why Google has the "It doesn't have to be perfect; it just needs to be better" standard: So reviewers can't impose undue costs on authors.
- mgraczyk 3y ago
  I fought with the review admins about this crap a lot when I was at Google. When you're working toward readability, code that isn't related to change is not in scope. The reviewer can comment on it but you're not expected to change it. That is in the official "rules".

by ayberk 3y ago

jgoodknight 3y ago

That was my experience getting Java readability too! Only 1 time in 20 did it feel like they were making the code better

tunesmith 3y ago

Getting reviews from multiple people that disagree on style is definitely an org problem that sucks to be in the middle of. However, I'd say that getting nit comments on code that surrounds your changes, but that you didn't change, is still fair game. It's part of the leave your campsite cleaner than you found it. It depends on the culture though - if it's suggested in the manner of "since you're here, here's an opportunity for how to improve this area of the code", that's better then acting like you made a mistake in failing to change it.

xKingfisher 3y ago
I try to avoid nits totally unrelated to the changes at hand, since on a subconscious level they may discourage people from even wanting to touch older/less loved files at all. The critical exception being avoiding issues due to path dependence. E.g while a change is "correct" is doing X poorly because of surrounding issue Y. So we should fix Y now instead of building atop it.
eichin 3y ago
Something I find helps with this in particular is only allowing style comments with citations to an actual style guide item. (I talk about domain-specific style guides as "crystallized arguments" - we agreed on this and wrote it down, not because it's necessarily right (though it probably is) but that we really wanted to stop wasting time arguing about these particular things.)
compiler-guy 3y ago
This can add cost and latency to the change author and distracts them from the project at hand. And because it costs the reviewer basically nothing to say, "While you are in there....", it slows the original author's work. Even worse, if there are enough of these minor changes, some other reviewer may take issue with their "small, focused changes" preference, and ask you to split it. This is why Google has the "It doesn't have to be perfect; it just needs to be better" standard: So reviewers can't impose undue costs on authors.
mgraczyk 3y ago
I fought with the review admins about this crap a lot when I was at Google. When you're working toward readability, code that isn't related to change is not in scope. The reviewer can comment on it but you're not expected to change it. That is in the official "rules".