4 ms·

Team conventions are not cargo culting. Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding s

Team conventions are not cargo culting. Change is bad unless it's great. Being able to look around a codebase and know how things work because similar coding styles and patterns are consistently followed is a huge productivity boost (this is also what commenters complaining about the idea of readability elsewhere in this thread are missing). Something must be 10x better to compensate for diverging from those patterns. What you write is not YOUR code. It is your TEAM'S code, and the good of the team is far more important than what you personally like.

withinboredom 3y ago
Heh, if that's your reasoning on why something should be the way it is, then that is what it is. Somewhat reasonable, but don't be surprised if any reasonable person quits that day. The argument is not grounded at all in computer science or anything else objectionable. It doesn't allow the team to grow and change what is in front of them every day and forces them to live with old mistakes forever. Doesn't sound like a good place to work. You don't get to a 10x solution overnight, in a single PR, you get there in increments. I also disagree with it being "the team's code": What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means). The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.
- Arainach 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. 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

Heh, if that's your reasoning on why something should be the way it is, then that is what it is. Somewhat reasonable, but don't be surprised if any reasonable person quits that day. The argument is not grounded at all in computer science or anything else objectionable. It doesn't allow the team to grow and change what is in front of them every day and forces them to live with old mistakes forever. Doesn't sound like a good place to work. You don't get to a 10x solution overnight, in a single PR, you get there in increments. I also disagree with it being "the team's code": What you write is your code (and copyright law almost universally backs this up), what gets merged is a maintenance burden for all time. It very much matters what you like and don't like, and it very much matters that it is maintainable (whatever that means). The team ... doesn't matter when it comes to code conventions ... they'll all be gone and moved on to other parts of the code/company/industry outside of five years. Most code lives long beyond today's team.

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