3 ms·

> Things like "I find this code very hard to follow, and I think it could be made easier" is also objective The "I" in that sentence suggests this should be co

> Things like "I find this code very hard to follow, and I think it could be made easier" is also objective The "I" in that sentence suggests this should be considered subjective. And that's I think the cleave-point between gating and non-gating: "Other people have already agreed on this" vs. "In the moment, I, a single code-reviewer, think this name could be improved."

arp242 3y ago
It's not an exact science and there's a grey area of course, but what I have in mind is mostly code that's just needlessly confusing in ways I think very few would disagree with. I can't really think of a specific non-trivial example at the moment, but just like natural language I do think some code can objectively "hard to follow", even if that's somewhat vague and not strictly defined. Other factors are if I'm the primary or one of the primary maintainers, the standing of the other person (also a maintainer or one-time contributor from another team), and things like that.
- shadowgovt 3y ago
  I can't speak to everyone's experience, but I can say how teams I've been on have handled that sort of thing. When it's not explicitly documented in the style guide and it doesn't violate some agreed-upon convention (which needs to be gleanable from within the file itself or it's not an agreed-upon convention... No "We know it was always done like that but now we're doing this," if you want to change it, you do the heavy lifting of putting together the consistency refactor to bloody well change it...), it's a suggestion. Non-blocking. But taste does matter, so if an engineer is observed by their peers to be ignoring too many refactor suggestions with no explanation, it becomes an issue with the project lead and that engineer might get put on a shorter leash (in the form of "These are normally non-blocking... Except for Steve, he wouldn't know consistent and terse naming if it bit him on the FaceManagerFactoryService). It required enough human touch to understand what each others' strengths and weaknesses were, so more expensive in terms of mind-space, but I think it generated better results overall. (And as the one whose code is reviewed: if you have a good reason not to change something and can express it, fine! You're a team member too and your opinion on how things should be also matters).
  - arp242 3y ago
    I'm not talking about style, I'm talking about logic. Some logic is easier to follow than others, and as mentioned this is not exact and somewhat fuzzy, but "every logic is equal to every other logic" is obviously nonsense. All other things being equal two "if" statements are better than nine "if" statements. I don't care about style; as far as I'm concerned it's "do what thou wilt shall be the whole of the style guide" (well, within some obvious limits of reason). I don't even care about consistency all that much.

arp242 3y ago

It's not an exact science and there's a grey area of course, but what I have in mind is mostly code that's just needlessly confusing in ways I think very few would disagree with. I can't really think of a specific non-trivial example at the moment, but just like natural language I do think some code can objectively "hard to follow", even if that's somewhat vague and not strictly defined. Other factors are if I'm the primary or one of the primary maintainers, the standing of the other person (also a maintainer or one-time contributor from another team), and things like that.

shadowgovt 3y ago
I can't speak to everyone's experience, but I can say how teams I've been on have handled that sort of thing. When it's not explicitly documented in the style guide and it doesn't violate some agreed-upon convention (which needs to be gleanable from within the file itself or it's not an agreed-upon convention... No "We know it was always done like that but now we're doing this," if you want to change it, you do the heavy lifting of putting together the consistency refactor to bloody well change it...), it's a suggestion. Non-blocking. But taste does matter, so if an engineer is observed by their peers to be ignoring too many refactor suggestions with no explanation, it becomes an issue with the project lead and that engineer might get put on a shorter leash (in the form of "These are normally non-blocking... Except for Steve, he wouldn't know consistent and terse naming if it bit him on the FaceManagerFactoryService). It required enough human touch to understand what each others' strengths and weaknesses were, so more expensive in terms of mind-space, but I think it generated better results overall. (And as the one whose code is reviewed: if you have a good reason not to change something and can express it, fine! You're a team member too and your opinion on how things should be also matters).
- arp242 3y ago
  I'm not talking about style, I'm talking about logic. Some logic is easier to follow than others, and as mentioned this is not exact and somewhat fuzzy, but "every logic is equal to every other logic" is obviously nonsense. All other things being equal two "if" statements are better than nine "if" statements. I don't care about style; as far as I'm concerned it's "do what thou wilt shall be the whole of the style guide" (well, within some obvious limits of reason). I don't even care about consistency all that much.