3 ms·

I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback

I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks. It's not like you're being told to never use for loops. Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against: * This logic should live in [other component] * Our RPCs are named as GetFoo, not DetermineFoo * That function name does not make it obvious what this code does, please change it * This needs a test * This is untestable and needs to be refactored to support X If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them.

withinboredom 3y ago
> I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks. Same. Nor do I consider code review comments personal attacks, but anything that must be changed for "reasons," must be questioned. Not because it's personal, but because I legitimately want to know and I won't give up until I get a good answer. > Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against I'll agree that none of this is probably worth pushing back against, but it isn't objective. Very little is objective, in our industry. > This logic should live in [other component] Why? DDD will say one thing, MVC will say something different, though usually they are compatible or can be made to be compatible. You can also subscribe to hexagonal architectures and that might say something different... There's nothing objective about where code should live, except on a hard drive or some other storage medium. I would argue that code probably shouldn't live on paper. I imagine most of us would agree with that. > Our RPCs are named as GetFoo, not DetermineFoo This is a nit. I'd honestly probably ignore it. > That function name does not make it obvious what this code does, please change it I'd probably ask for a suggestion because it probably looks obvious to me after staring at the code for so long. That being said, it might be a valid suggestion, especially if the code was refactored, but wasn't renamed. > This needs a test I hope nobody ever says this on my code reviews. However, I don't write tests for "obviously correct" code (code where the test implements the logic to test the logic): such as a function like: function returnTrue(): true { return true; } If I see code that changes "obviously correct" code, then a test is warranted. > This is untestable and needs to be refactored to support X Do you know there is a such thing as legitimately untestable code (or at least, it shouldn't be tested in traditional unit tests)? Usually at the edges of two systems. For example, an API integration can't be tested fully, only the known contract from the other system. Then you start running into Postel's Law ... things get weird. Only if you have some kinds of guarantees with the other system (not usually), would I recommend traditional tests.
- Arainach 3y ago
  >> Our RPCs are named as GetFoo, not DetermineFoo >This is a nit. I'd honestly probably ignore it. Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused. "The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers. As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.
  - withinboredom 3y ago
    > until the end of time your team now has to remember multiple naming conventions Do they not have PR's where you are from? If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa. The code is mutable. It doesn't have to be perfect on the first iteration. I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.
    - Arainach 3y ago
      The code is mutable, but the cost to fix it during the original code review is WAY lower. Once you land RPC DescribeFoo, I have to: 1) Define a second parallel RPC GetFoo 2) Wait for it to roll out to prod 3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover 4) Wait for all of those to roll out 5) Clean up all the callers 6) Clean up the DescribeFoo RPC .....or you could just fix it in code review with half an hour of work.
      - 3y ago
withinboredom 3y ago
> If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them. I wasn't sure if I was going to reply to this part or not. But you took a comment out of context here. I'm incredibly touchy about code reviews. I used to have the popular opinion here where "the code belongs to the team!" and all that. And then I was stuck on a team with a very toxic guy who weaponized this stuff. His code was "perfect" and he'd approve your PR to turn around and rewrite it just to show how much better it could be (and gloat about it -- not realizing if he hadn't deleted half the tests, they'd be failing). He'd constantly point out all the things that were "wrong" and with the justification of "code quality" without actually being able to point out why it was better. After working with them for two years, the entire team's philosophy turned into the one I've shared here ... because it can't be weaponized. It can't be used to tyrannically rule the code. It can't be used to bully people around "for the sake of quality!" His "code quality tyranny" did more damage to the code than any actually bad code could have done. So yeah, I will go into a war of attrition over certain kinds of comments that are spoken authoritatively yet are entirely opinionated ... because I've seen what happens when it gets out of hand. I have no desire to go through that ever again. Ever. If that means someone like you fires me for it, it's better than going through that shit again. Sorry, not sorry.
- redditor98654 3y ago
  You experience one extreme ans then decided to take the opposite extreme stance. I recommend being more pragmatic.
  - withinboredom 3y ago
    Eventually, sure. I'm also starting to think this is how the Paradox of Tolerance happens. I'm intolerant of intolerance these days. I'm much more tolerant of people having their way of doing things than I was just 10 years ago. However, as soon as one person comes along and says "It MUST be this way because QUALITY!" I go into intolerance mode and don't tolerate it. Now, if you can give me a reason grounded in computer science, or whatever paradigm we're using (DDD, hexagonal arch, actor-model, etc), then I'm perfectly happy to agree or spend a few minutes over a coffee discussing merits. But if the reason is 'quality' ... it's a "dual to the death."

withinboredom 3y ago

> I've worked with some amazing engineers, including multiple whose blog posts regularly get posted here. None of them have egos or consider code review feedback personal attacks. Same. Nor do I consider code review comments personal attacks, but anything that must be changed for "reasons," must be questioned. Not because it's personal, but because I legitimately want to know and I won't give up until I get a good answer. > Code review feedback such as the following is all objective, beneficial to the reader, and not worth pushing back against I'll agree that none of this is probably worth pushing back against, but it isn't objective. Very little is objective, in our industry. > This logic should live in [other component] Why? DDD will say one thing, MVC will say something different, though usually they are compatible or can be made to be compatible. You can also subscribe to hexagonal architectures and that might say something different... There's nothing objective about where code should live, except on a hard drive or some other storage medium. I would argue that code probably shouldn't live on paper. I imagine most of us would agree with that. > Our RPCs are named as GetFoo, not DetermineFoo This is a nit. I'd honestly probably ignore it. > That function name does not make it obvious what this code does, please change it I'd probably ask for a suggestion because it probably looks obvious to me after staring at the code for so long. That being said, it might be a valid suggestion, especially if the code was refactored, but wasn't renamed. > This needs a test I hope nobody ever says this on my code reviews. However, I don't write tests for "obviously correct" code (code where the test implements the logic to test the logic): such as a function like: function returnTrue(): true { return true; } If I see code that changes "obviously correct" code, then a test is warranted. > This is untestable and needs to be refactored to support X Do you know there is a such thing as legitimately untestable code (or at least, it shouldn't be tested in traditional unit tests)? Usually at the edges of two systems. For example, an API integration can't be tested fully, only the known contract from the other system. Then you start running into Postel's Law ... things get weird. Only if you have some kinds of guarantees with the other system (not usually), would I recommend traditional tests.

Arainach 3y ago
>> Our RPCs are named as GetFoo, not DetermineFoo >This is a nit. I'd honestly probably ignore it. Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused. "The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers. As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.
- withinboredom 3y ago
  > until the end of time your team now has to remember multiple naming conventions Do they not have PR's where you are from? If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa. The code is mutable. It doesn't have to be perfect on the first iteration. I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.
  - Arainach 3y ago
    The code is mutable, but the cost to fix it during the original code review is WAY lower. Once you land RPC DescribeFoo, I have to: 1) Define a second parallel RPC GetFoo 2) Wait for it to roll out to prod 3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover 4) Wait for all of those to roll out 5) Clean up all the callers 6) Clean up the DescribeFoo RPC .....or you could just fix it in code review with half an hour of work.
    - 3y ago

withinboredom 3y ago

> If anyone on my team ever described themselves as in "a war of attrition" with another teammate I'd fire them. I wasn't sure if I was going to reply to this part or not. But you took a comment out of context here. I'm incredibly touchy about code reviews. I used to have the popular opinion here where "the code belongs to the team!" and all that. And then I was stuck on a team with a very toxic guy who weaponized this stuff. His code was "perfect" and he'd approve your PR to turn around and rewrite it just to show how much better it could be (and gloat about it -- not realizing if he hadn't deleted half the tests, they'd be failing). He'd constantly point out all the things that were "wrong" and with the justification of "code quality" without actually being able to point out why it was better. After working with them for two years, the entire team's philosophy turned into the one I've shared here ... because it can't be weaponized. It can't be used to tyrannically rule the code. It can't be used to bully people around "for the sake of quality!" His "code quality tyranny" did more damage to the code than any actually bad code could have done. So yeah, I will go into a war of attrition over certain kinds of comments that are spoken authoritatively yet are entirely opinionated ... because I've seen what happens when it gets out of hand. I have no desire to go through that ever again. Ever. If that means someone like you fires me for it, it's better than going through that shit again. Sorry, not sorry.

redditor98654 3y ago
You experience one extreme ans then decided to take the opposite extreme stance. I recommend being more pragmatic.
- withinboredom 3y ago
  Eventually, sure. I'm also starting to think this is how the Paradox of Tolerance happens. I'm intolerant of intolerance these days. I'm much more tolerant of people having their way of doing things than I was just 10 years ago. However, as soon as one person comes along and says "It MUST be this way because QUALITY!" I go into intolerance mode and don't tolerate it. Now, if you can give me a reason grounded in computer science, or whatever paradigm we're using (DDD, hexagonal arch, actor-model, etc), then I'm perfectly happy to agree or spend a few minutes over a coffee discussing merits. But if the reason is 'quality' ... it's a "dual to the death."