3 ms·

> 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

> 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."
  - redditor98654 3y ago
    I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority. Here is the most recent PR I did for a relatively young person in my org. Part of the PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly. I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing". Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?
    - withinboredom 3y ago
      There are three parts to every code review: 1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that). 2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better? 3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone. In this particular case: Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library. If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it. The work has already been done. Try to capitalize on it, instead of dismissing it. Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.

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."
- redditor98654 3y ago
  I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority. Here is the most recent PR I did for a relatively young person in my org. Part of the PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly. I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing". Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?
  - withinboredom 3y ago
    There are three parts to every code review: 1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that). 2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better? 3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone. In this particular case: Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library. If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it. The work has already been done. Try to capitalize on it, instead of dismissing it. Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.