3 ms·

> a list comprehension or a map is just nitpicking Absolutely. It’s a style choice. Same with the variable names. Ironically the whole article is a great examp

by klabb3 2y ago

> a list comprehension or a map is just nitpicking Absolutely. It’s a style choice. Same with the variable names. Ironically the whole article is a great example of what a waste of time code reviews have become. If style needs to be consistent, it should be enforced with a style guide or ideally automated formatting tools. If it’s not in the style guide, you can bike shed it in a separate meeting, but don’t block a PR on this crap. If you’re submitting a PR to an OSS project as an outsider, then I can understand more that authors want discretions. A pattern I like is where a maintainer cleans up stylistic things and then just says thanks and commits. In either case, the overall priority should be to reduce unnecessary round trips.

thiht 2y ago
> Same with the variable names No. Code is harder to read than it is to write. It's also read countless of times. Obviously single letter variables are ok for short scoped variables, or for conventional names[1] but other than that I will definitely ask for a clearer variable name. I will also ask to write full named variables (eg. user instead of usr), it's easier to read and less ambiguous in some cases. Variable names can make all the difference between unreadable code and completely fine code. [1]: For example I write Go, and in my Services layer the pointer receiver (akin to "this" in other languages) is always named "s" for convenience. And in handlers the pointer receiver is always "h".
- klabb3 2y ago
  Yeah sure but would you spend a round trip and block it? It’s not hard to discuss coding processes but this constant going back and forth asynchronously is dreadful. People get notification damaged by this.
  - thiht 2y ago
    > would you spend a round trip and block it Yes. If I spend time reviewing some code and I don’t understand some part of it because of naming, it’s definitely an issue that must be addressed during the code review. You just need to learn not to take it personally, reviewing is literally a third party coming to review your code without your biases, so they benefit from the "fresh eye". You should listen, because in 3 months you’ll be the fresh eyes trying to understand the code you wrote 3 months ago, cursing yourself for not using more descriptive names.

by klabb3 2y ago

thiht 2y ago

> Same with the variable names No. Code is harder to read than it is to write. It's also read countless of times. Obviously single letter variables are ok for short scoped variables, or for conventional names[1] but other than that I will definitely ask for a clearer variable name. I will also ask to write full named variables (eg. user instead of usr), it's easier to read and less ambiguous in some cases. Variable names can make all the difference between unreadable code and completely fine code. [1]: For example I write Go, and in my Services layer the pointer receiver (akin to "this" in other languages) is always named "s" for convenience. And in handlers the pointer receiver is always "h".

klabb3 2y ago
Yeah sure but would you spend a round trip and block it? It’s not hard to discuss coding processes but this constant going back and forth asynchronously is dreadful. People get notification damaged by this.
- thiht 2y ago
  > would you spend a round trip and block it Yes. If I spend time reviewing some code and I don’t understand some part of it because of naming, it’s definitely an issue that must be addressed during the code review. You just need to learn not to take it personally, reviewing is literally a third party coming to review your code without your biases, so they benefit from the "fresh eye". You should listen, because in 3 months you’ll be the fresh eyes trying to understand the code you wrote 3 months ago, cursing yourself for not using more descriptive names.