3 ms·

At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it. I think the key thing is t

by arp242 3y ago

At not-Google-scale code also often lasts for years; perhaps even more so since there's typically a lot fewer people maintaining it. I think the key thing is to ask yourself "is this really objectively better yes/no?" before commenting. Not that you can never comment if the answer to that is "no", but quite a lot of the time when the answer is "no" it doesn't really matter and it's just "I would have done it slightly different, but both are fine".

shadowgovt 3y ago
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.
  - 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).

by arp242 3y ago

shadowgovt 3y ago

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.
- 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).