3 ms·

Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) ma

Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.

arp242 3y ago
But "another team is relying on not to move" is an objective point, right? Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer". I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone pointed out that "dir" was a needless abbreviation so it became createDirectory(). Then yet someone else pointed out that it's actually recursive, and a discussion on the merits of createDirectoryRecursively() vs. createRecursiveDirectory() was inflicted on everyone. In-between there was a side-quest about directory vs. folder. Just fucking stick a fork in my eye already. Was anything of any objective value gained? I'm having a hard time seeing it. Well, it makes for a slightly amusing anecdote so there's that.
- ryandrake 3y ago
  I'd wonder what the rest of that API surface looked like. If the rest of the functions were longDescriptiveCamelCase() and then you tried to slip in mkdir(), then I'd say yes, "stylistic consistency" was gained during that unnecessarily difficult back-and-forth. If the other functions were chdir(), remove(), rmdir() and so on, and somehow the reviewers picked your code change as the time to change it all, then yea, what a waste.
  - arp242 3y ago
    It was a mix of short and long and there wasn't really much "style". The company had taken over the maintainership from someone else at some point (before I joined) and it was all fairly inconsistent. I don't really mind the inconsistencies as such, it's just the argueing over nothing that I mind. This kind of stuff was typical. At some point there was a lengthy discussion which prevented rolling out a rather important hotfix over: while (true) { if (someCond()) break; if (otherCond()) break; [..] } It wasn't even about whether someCond() and otherCond() should be in that while(..) condition, it was allegedly "dangerous" because it "could loop infinitely". Well, ehh, that's the case with any lop innit? That's kind of how they work? There was a bunch of instances of code like this, "but it's still dangerous". Hmkay... Oh, and then there was the great "evil incident". I had rewritten much of the frontend to this newfangled thing called "jQuery" that was all the rage. That was all fine and worked pretty well, but suddenly there was a bug hubhub about the jslint comment; the keyword to allow eval() was "evil" (one of those not-too-funny Crockford jokes), so at the top of /static/js/app.minified.js?v=12311 in prodiction there was something like: /*! jslint: keyword1 evil keyword2 */ /*! MIT license blah blah */ minified_js()... And people were up in arms and there was a big panic deploy during lunchtime for this "because customers might see this, and it's highly unprofessional, and it's a big problem we need to fix ASAP" etc. etc. etc. This was in the Netherlands with regular people, not some highly conservative part of the world. It was downright surreal and bizarre. What I'm trying to say is that these people were rather obsessed with minor details to a point I've never seen before or since. Hell, there was a Company Blessed IDE™ that you had to use. Nothing else allowed. It was a complete piece of shit (IMHO) and after a few weeks I just used Vim. It worked. I got stuff done. No one was bothered by it. Still got comments I shouldn't be doing that... While I didn't formulate "is this really objectively better yes/no?" clearly at that time, I'm sure it's been a pretty big influence.
    - IanCal 3y ago
      > it was allegedly "dangerous" because it "could loop infinitely". Well, ehh, that's the case with any lop innit? That's kind of how they work? No? You can clearly have loops that exit. Loops that can be infinite should only be so where you really want that to happen in those cases / are fine with it. "Infinitely until the user does X with no timeout" is a fair one.
      - arp242 3y ago
        Every while/for loop is essentially "keep doing this until I tell you to stop". You can argue what the best way to "tell to stop" is, but that doesn't really change the concept. No one could even articulate under which conditions this could happen. Cosmic rays altering memory? That's the kind of stuff we're talking about here. For a PHP webapp. I don't recall the exact specifics but we spent a long time on that stupid loop and no one even had any alternative. Eventually the loop committed. The thing is, if I had written the conditions in the loop body it would have been fine with no comments. People were tripped over the "while (true)" and commented on that. That's okay, I can just change that, no problem. But the kept analysing and thinking about it to the subatomic detail. That as really the problem: small things tended to escalate to "a big thing" even when it was just a small thing. I don't know what it was – a kind tunnel vision? I don't know. I once carefully suggested to paint a wall in a brighter colour during a HOA meeting. People objected. Fine, not a big deal and just a suggestion, I'm okay with the grey it was. They kept discussing it. I back-pedalled harder. People started to suggest we need to form a committee. I started to run. They asked if I wanted to join the committee "as I had brought up the issue". I overtook Usain Bolt. I heard they had several meetings. They choose the same grey it was before (or something so similar I couldn't spot the difference). Not my favourite colour or what I would have chosen, but it's fine.
        joshuamorton 3y ago
        I can prove, pretty trivially that for i in range(10): Will halt. Slightly less trivially, you can prove that while true: if x: break else: break Will halt. But once the if conditions aren't exhaustive, it becomes difficult, perhaps impossible, to know that you won't end up in a weird unhandled state. Preferring and even insisting upon code that is not only possible, but easy, to reason about is good. Or iow it should be incumbent on you to prove the loop halts, not on the reviewers to prove it doesn't.
- shadowgovt 3y ago
  > 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).

Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.

arp242 3y ago

But "another team is relying on not to move" is an objective point, right? Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer". I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone pointed out that "dir" was a needless abbreviation so it became createDirectory(). Then yet someone else pointed out that it's actually recursive, and a discussion on the merits of createDirectoryRecursively() vs. createRecursiveDirectory() was inflicted on everyone. In-between there was a side-quest about directory vs. folder. Just fucking stick a fork in my eye already. Was anything of any objective value gained? I'm having a hard time seeing it. Well, it makes for a slightly amusing anecdote so there's that.

ryandrake 3y ago
I'd wonder what the rest of that API surface looked like. If the rest of the functions were longDescriptiveCamelCase() and then you tried to slip in mkdir(), then I'd say yes, "stylistic consistency" was gained during that unnecessarily difficult back-and-forth. If the other functions were chdir(), remove(), rmdir() and so on, and somehow the reviewers picked your code change as the time to change it all, then yea, what a waste.
- arp242 3y ago
  It was a mix of short and long and there wasn't really much "style". The company had taken over the maintainership from someone else at some point (before I joined) and it was all fairly inconsistent. I don't really mind the inconsistencies as such, it's just the argueing over nothing that I mind. This kind of stuff was typical. At some point there was a lengthy discussion which prevented rolling out a rather important hotfix over: while (true) { if (someCond()) break; if (otherCond()) break; [..] } It wasn't even about whether someCond() and otherCond() should be in that while(..) condition, it was allegedly "dangerous" because it "could loop infinitely". Well, ehh, that's the case with any lop innit? That's kind of how they work? There was a bunch of instances of code like this, "but it's still dangerous". Hmkay... Oh, and then there was the great "evil incident". I had rewritten much of the frontend to this newfangled thing called "jQuery" that was all the rage. That was all fine and worked pretty well, but suddenly there was a bug hubhub about the jslint comment; the keyword to allow eval() was "evil" (one of those not-too-funny Crockford jokes), so at the top of /static/js/app.minified.js?v=12311 in prodiction there was something like: /*! jslint: keyword1 evil keyword2 */ /*! MIT license blah blah */ minified_js()... And people were up in arms and there was a big panic deploy during lunchtime for this "because customers might see this, and it's highly unprofessional, and it's a big problem we need to fix ASAP" etc. etc. etc. This was in the Netherlands with regular people, not some highly conservative part of the world. It was downright surreal and bizarre. What I'm trying to say is that these people were rather obsessed with minor details to a point I've never seen before or since. Hell, there was a Company Blessed IDE™ that you had to use. Nothing else allowed. It was a complete piece of shit (IMHO) and after a few weeks I just used Vim. It worked. I got stuff done. No one was bothered by it. Still got comments I shouldn't be doing that... While I didn't formulate "is this really objectively better yes/no?" clearly at that time, I'm sure it's been a pretty big influence.
  - IanCal 3y ago
    > it was allegedly "dangerous" because it "could loop infinitely". Well, ehh, that's the case with any lop innit? That's kind of how they work? No? You can clearly have loops that exit. Loops that can be infinite should only be so where you really want that to happen in those cases / are fine with it. "Infinitely until the user does X with no timeout" is a fair one.
    - arp242 3y ago
      Every while/for loop is essentially "keep doing this until I tell you to stop". You can argue what the best way to "tell to stop" is, but that doesn't really change the concept. No one could even articulate under which conditions this could happen. Cosmic rays altering memory? That's the kind of stuff we're talking about here. For a PHP webapp. I don't recall the exact specifics but we spent a long time on that stupid loop and no one even had any alternative. Eventually the loop committed. The thing is, if I had written the conditions in the loop body it would have been fine with no comments. People were tripped over the "while (true)" and commented on that. That's okay, I can just change that, no problem. But the kept analysing and thinking about it to the subatomic detail. That as really the problem: small things tended to escalate to "a big thing" even when it was just a small thing. I don't know what it was – a kind tunnel vision? I don't know. I once carefully suggested to paint a wall in a brighter colour during a HOA meeting. People objected. Fine, not a big deal and just a suggestion, I'm okay with the grey it was. They kept discussing it. I back-pedalled harder. People started to suggest we need to form a committee. I started to run. They asked if I wanted to join the committee "as I had brought up the issue". I overtook Usain Bolt. I heard they had several meetings. They choose the same grey it was before (or something so similar I couldn't spot the difference). Not my favourite colour or what I would have chosen, but it's fine.
      - joshuamorton 3y ago
        I can prove, pretty trivially that for i in range(10): Will halt. Slightly less trivially, you can prove that while true: if x: break else: break Will halt. But once the if conditions aren't exhaustive, it becomes difficult, perhaps impossible, to know that you won't end up in a weird unhandled state. Preferring and even insisting upon code that is not only possible, but easy, to reason about is good. Or iow it should be incumbent on you to prove the loop halts, not on the reviewers to prove it doesn't.
shadowgovt 3y ago
> 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).