7 ms·

I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not

I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.

shadowgovt 3y ago
It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that will be thrown away in six months. And there's a big difference between the code that makes up an API layer and the code that implements a feature constrained to one module.
- 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.
TheBlight 3y ago
Excessive subjective feedback can be soul-crushing.
- acscott 3y ago
  Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development
  - TheBlight 3y ago
    It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
  - wubrr 3y ago
    The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.
    - runlevel1 3y ago
      That seems like it risks creating conflict out of what's often just a misunderstanding. Assuming it's a corporate environment (it's fuzzier in the open source bazaar): If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see how reasonable they are, and can get clarification of what they actually care about versus FYIs/suggestions. As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. If everything is a hard opinion and they don't seem reasonable, I reach out to someone else (ideally a team lead or peer on their team) over a private channel for a 2nd opinion. If they also think it's unimportant stuff, I ask them to add their own comments to the PR. Give it a reasonable amount of time and they'll either have reached a consensus or you can roll the side you agree with. If it's an issue again later and they seem reasonable, respectfully push back. If they seem unreasonable, skip right to DMing their lead for a 2nd opinion. If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. Of course, if you're at a company with a lot of disfunction, well... I can understand why what I've written above won't work.
jeffbee 3y ago
Writing readable code is not "nitpicking". It is common that an author doesn't see why their parameter name or function name is misleading but their reviewer, who comes to the change without as many preconceptions, sees it right away.
- dontreact 3y ago
  Im not saying it’s always nitpicking. I’m saying that sometimes it is.
- bheadmaster 3y ago
  "Readable" and "misleading" are subjective, and depend on the person's "preconceptions". I disagree that the reviewer comes "without as many preconceptions" - they just come with different preconceptions, the ones they're used to. Programming language is a language like any other, and each person has their own style of writing it, and they'd prefer the rest of the world to use their own style because it's "more readable".
  - tikhonj 3y ago
    Are you saying there is no such thing as clearer or less clear writing in natural languages or programming languages? It's 100% subjective and depends entirely on the reader?
    - bheadmaster 3y ago
      Of course, it's not 100% subjective - rarely anything in the world is 100% anything. But I think that most people consider their personal preferences to be better and more readable just because they're used to them, so I tend to take the opposite attitude as the starting point. There were more than a few situations where I've had a coworker tell me "just read the code, it's very readable", only to spend the next two weeks just trying to figure out how it works. Sure, once you figure out how it works and it "clicks", it's no longer (that) unreadable, but the fact that I have to spend so much time reading the codebase in the first place made me convinced that personal familiarity is a great part of what "readable" means.
seanmcdirmid 3y ago
you can mark your comment (in text) as a NIT and then unmark "Action required."
- dontreact 3y ago
  Yeah… but people sometimes dont do this
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 :)
- 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.)
egl2021 3y ago
Different languages had different pools of readability reviewers, so the expectations varied, but readability reviews were generally constructive and helpful. I was thrilled to have Ian Taylor review my go code. The non-readability reviewers were usually on your team, so there was social pressure in both directions. You wanted to learn and conform to the team's norms, and the reviewer couldn't be a total jerk about their comments. Everyone was generally on their best behavior.
Tyr42 3y ago
At least with the AI assist it's easy to one click accept the name change and be done with it.
chii 3y ago
> Sometimes it’s not worth arguing over small details like variable naming but naming is quite important. May be it's not that this is a nitpick, but that previous review tools prevent the fruitful discussion of names.

shadowgovt 3y ago

It can be tricky to find a balance point here. At Google scale (in time-of-maintenance, not space... Code can last for years and will be worked on by multiple engineers disconnected from the original project), the hard problem of naming things becomes real. I find that it's useful to keep context. Even though one can never predict with certainty, sometimes you can be real confident that some code is prototype that will be thrown away in six months. And there's a big difference between the code that makes up an API layer and the code that implements a feature constrained to one module.

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.

TheBlight 3y ago

Excessive subjective feedback can be soul-crushing.

acscott 3y ago
Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development
- TheBlight 3y ago
  It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
- wubrr 3y ago
  The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.
  - runlevel1 3y ago
    That seems like it risks creating conflict out of what's often just a misunderstanding. Assuming it's a corporate environment (it's fuzzier in the open source bazaar): If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see how reasonable they are, and can get clarification of what they actually care about versus FYIs/suggestions. As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. If everything is a hard opinion and they don't seem reasonable, I reach out to someone else (ideally a team lead or peer on their team) over a private channel for a 2nd opinion. If they also think it's unimportant stuff, I ask them to add their own comments to the PR. Give it a reasonable amount of time and they'll either have reached a consensus or you can roll the side you agree with. If it's an issue again later and they seem reasonable, respectfully push back. If they seem unreasonable, skip right to DMing their lead for a 2nd opinion. If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. Of course, if you're at a company with a lot of disfunction, well... I can understand why what I've written above won't work.

jeffbee 3y ago

Writing readable code is not "nitpicking". It is common that an author doesn't see why their parameter name or function name is misleading but their reviewer, who comes to the change without as many preconceptions, sees it right away.

dontreact 3y ago
Im not saying it’s always nitpicking. I’m saying that sometimes it is.
bheadmaster 3y ago
"Readable" and "misleading" are subjective, and depend on the person's "preconceptions". I disagree that the reviewer comes "without as many preconceptions" - they just come with different preconceptions, the ones they're used to. Programming language is a language like any other, and each person has their own style of writing it, and they'd prefer the rest of the world to use their own style because it's "more readable".
- tikhonj 3y ago
  Are you saying there is no such thing as clearer or less clear writing in natural languages or programming languages? It's 100% subjective and depends entirely on the reader?
  - bheadmaster 3y ago
    Of course, it's not 100% subjective - rarely anything in the world is 100% anything. But I think that most people consider their personal preferences to be better and more readable just because they're used to them, so I tend to take the opposite attitude as the starting point. There were more than a few situations where I've had a coworker tell me "just read the code, it's very readable", only to spend the next two weeks just trying to figure out how it works. Sure, once you figure out how it works and it "clicks", it's no longer (that) unreadable, but the fact that I have to spend so much time reading the codebase in the first place made me convinced that personal familiarity is a great part of what "readable" means.

seanmcdirmid 3y ago

you can mark your comment (in text) as a NIT and then unmark "Action required."

dontreact 3y ago
Yeah… but people sometimes dont do this

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

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

egl2021 3y ago

Different languages had different pools of readability reviewers, so the expectations varied, but readability reviews were generally constructive and helpful. I was thrilled to have Ian Taylor review my go code. The non-readability reviewers were usually on your team, so there was social pressure in both directions. You wanted to learn and conform to the team's norms, and the reviewer couldn't be a total jerk about their comments. Everyone was generally on their best behavior.

Tyr42 3y ago

At least with the AI assist it's easy to one click accept the name change and be done with it.

chii 3y ago

> Sometimes it’s not worth arguing over small details like variable naming but naming is quite important. May be it's not that this is a nitpick, but that previous review tools prevent the fruitful discussion of names.