4 ms·

That example about the for loop... I think the author is getting the entirely wrong lesson there. The best code review would just be something like "please use

That example about the for loop... I think the author is getting the entirely wrong lesson there. The best code review would just be something like "please use more descriptive variable names". Whether you use a for loop, a list comprehension or a map is just nitpicking, it has almost zero effect on maintainability. The best code review is the one which does not spend its time on stylistic nitpicks but focuses on the overall architecture of the change and the assumptions of the changed code. Getting the assumptions or the abstractions wrong has a much severe impact in future maintainability than any kind of stylistic deviation will ever have.

jeremyjh 2y ago
That should be the approach if you already working in a kitchen-sink project where every possible way of doing the same thing already has several examples, but if the project currently has 0 for loops in the codebase, I think you can say that is a norm for that project that should be upheld. (By the way, I'd often prefer for loops myself). Good developers will generally pick up a lot of norms from the existing code, and will even read style guides but some people always write code the same way regardless of the language or project they are working in, and I think that sort of feedback is appropriate for them. But I also think waaaay too much time was spent on the "Good Review". I'd have just said - "in this project we use comprehensions instead of for loops".
rzwitserloot 2y ago
The gist you're trying to convey is fine: Prioritize those things that _directly_ cause meaningful damage to maintainability. But a code base that is stylistically inconsistent is itself a source of unmaintainability. That's probably a case of "perfection is the enemy of good enough"; a multi-year, multi-member project perhaps cannot reasonably be expected to consistently pick e.g. a list comprehension instead of a for loop under similar circumstances. However, just stating casually that it is _irrelevant_, or that it is hopeless, feels like giving up a bit too quickly. One would assume that the cost to morale etc is overblown (if your team treats every comment as a minor 'error' that was caught, or worse, gets defensive about them - yes, _of course_. The solution is to tell the team that a directive suggestion in a code review simply isn't a complaint, error, oversight, or otherwise to be taken as negative feedback in any way. Code review is a process, it has multiple goals). Furthermore, if you spend the time to try it, presumably the team will coalesce, and the frequency of such comments will decrease. In practice a pointless style fight might break out where one side of the team insists 'situations like these should use for loops, not comprehensions!' and another side vehemently argues for the opposite. At which point, yes, that is obviously an extremely bad outcome. But perhaps _that_ is the right time to throw in the towel and agree together to allow a modicum of personal preference. Of course, if there are all sorts of maintainability issues, by all means, _do not_ spend much time on fighting such esoteric style battles at all. But not all teams are dysfunctional :)
- jt2190 2y ago
  > But a code base that is stylistically inconsistent is itself a source of unmaintainability Can you elaborate? The context here is a few lines of code that iterate over a list. I don’t know any developer who understands one that can’t understand the other.
  - shagie 2y ago
    A codebase where some developers use spaces, another use tabs. Changes to the other person's code always has whitespace changes on large blocks of text that are otherwise unmodified resulting in much more in a code review than is needed. One developer does simple for loops without {} and another always puts them in everywhere. When the second developer touches the block of code, there's a large amount of {} being added with no other functional changes in the block. One developer uses guard statements at the start of a method, the other always writes single exit point code. --- Changing between these or having two different styles within the same code base makes it more difficult to maintain it since there are non-functional changes mixed in with the functional changes. Its not a "can't understand the other" but rather "inconsistent style results in non-functional changes in the code as style changes are being made - making it harder to maintain the code and review it."
    - jt2190 2y ago
      This mostly describes anti-social developer behavior I think, someone who reformats code for no reason? As their manager why aren’t you taking this up directly with them? Why aren’t you helping them configure their editor correctly?
      - tracker1 2y ago
        Code formatting should be able to be applied from CLI as a pre-commit hook IMO. Relying on a specific plugin/configuration and specific editor/ide is a mistake and will lead to pain in the future. For example a VS only plugin as you take a project to add cross-platform support and onboard developers using say WSL or MacOS as opposed to JUST windows.
bluGill 2y ago
> Whether you use a for loop, a list comprehension or a map is just nitpicking, it has almost zero effect on maintainability. It makes a big difference in languages where for loops are subject to off by one errors. Also many libraries have algorithms for what you do with the list comprehension. If you have one function ApplySomeAlgorithmToEntireList(list) that is a lot clearer than trying to find what you are doing in the body of the for/list comprehension.
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.
agentultra 2y ago
I’m with you. The presumption to “educate,” the author is annoying and tedious. Unless the author asks for an opinion assume they know what they’re doing. Same goes for review “comments,” that interrogate the author. “Why did you choose to use a for loop here?” is such a waste of time. I agree, stay within stylistic guidelines and lacking any try to use the same style as the file being edited. And just focus on the code itself: does it meet the specifications, is it clear that it does what it’s supposed to, are all the T’s crossed and I’s dotted? Good, go. Brevity is it’s own form of clarity when used well. Update ... and remember to leave your ego at the door! Talk about the code like an object of study.
tapanjk 2y ago
One of the tendencies I see in reviews is that the reviewer focuses on the wrong stuff. For e.g. when reviewing the design document, they focus on the validity of the requirements and they may even stray into commenting on formatting and grammar in the document; when reviewing code, they focus on design; when reviewing test cases, again they focus on reviewing requirements; when reviewing deployment scripts, they focus on the choise of infrastructure. Basically, people tend to pay attention to things that are already past the decision stage while ignoring the matter at hand.

jeremyjh 2y ago

That should be the approach if you already working in a kitchen-sink project where every possible way of doing the same thing already has several examples, but if the project currently has 0 for loops in the codebase, I think you can say that is a norm for that project that should be upheld. (By the way, I'd often prefer for loops myself). Good developers will generally pick up a lot of norms from the existing code, and will even read style guides but some people always write code the same way regardless of the language or project they are working in, and I think that sort of feedback is appropriate for them. But I also think waaaay too much time was spent on the "Good Review". I'd have just said - "in this project we use comprehensions instead of for loops".

rzwitserloot 2y ago

The gist you're trying to convey is fine: Prioritize those things that _directly_ cause meaningful damage to maintainability. But a code base that is stylistically inconsistent is itself a source of unmaintainability. That's probably a case of "perfection is the enemy of good enough"; a multi-year, multi-member project perhaps cannot reasonably be expected to consistently pick e.g. a list comprehension instead of a for loop under similar circumstances. However, just stating casually that it is _irrelevant_, or that it is hopeless, feels like giving up a bit too quickly. One would assume that the cost to morale etc is overblown (if your team treats every comment as a minor 'error' that was caught, or worse, gets defensive about them - yes, _of course_. The solution is to tell the team that a directive suggestion in a code review simply isn't a complaint, error, oversight, or otherwise to be taken as negative feedback in any way. Code review is a process, it has multiple goals). Furthermore, if you spend the time to try it, presumably the team will coalesce, and the frequency of such comments will decrease. In practice a pointless style fight might break out where one side of the team insists 'situations like these should use for loops, not comprehensions!' and another side vehemently argues for the opposite. At which point, yes, that is obviously an extremely bad outcome. But perhaps _that_ is the right time to throw in the towel and agree together to allow a modicum of personal preference. Of course, if there are all sorts of maintainability issues, by all means, _do not_ spend much time on fighting such esoteric style battles at all. But not all teams are dysfunctional :)

jt2190 2y ago
> But a code base that is stylistically inconsistent is itself a source of unmaintainability Can you elaborate? The context here is a few lines of code that iterate over a list. I don’t know any developer who understands one that can’t understand the other.
- shagie 2y ago
  A codebase where some developers use spaces, another use tabs. Changes to the other person's code always has whitespace changes on large blocks of text that are otherwise unmodified resulting in much more in a code review than is needed. One developer does simple for loops without {} and another always puts them in everywhere. When the second developer touches the block of code, there's a large amount of {} being added with no other functional changes in the block. One developer uses guard statements at the start of a method, the other always writes single exit point code. --- Changing between these or having two different styles within the same code base makes it more difficult to maintain it since there are non-functional changes mixed in with the functional changes. Its not a "can't understand the other" but rather "inconsistent style results in non-functional changes in the code as style changes are being made - making it harder to maintain the code and review it."
  - jt2190 2y ago
    This mostly describes anti-social developer behavior I think, someone who reformats code for no reason? As their manager why aren’t you taking this up directly with them? Why aren’t you helping them configure their editor correctly?
    - tracker1 2y ago
      Code formatting should be able to be applied from CLI as a pre-commit hook IMO. Relying on a specific plugin/configuration and specific editor/ide is a mistake and will lead to pain in the future. For example a VS only plugin as you take a project to add cross-platform support and onboard developers using say WSL or MacOS as opposed to JUST windows.

bluGill 2y ago

> Whether you use a for loop, a list comprehension or a map is just nitpicking, it has almost zero effect on maintainability. It makes a big difference in languages where for loops are subject to off by one errors. Also many libraries have algorithms for what you do with the list comprehension. If you have one function ApplySomeAlgorithmToEntireList(list) that is a lot clearer than trying to find what you are doing in the body of the for/list comprehension.

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.

agentultra 2y ago

I’m with you. The presumption to “educate,” the author is annoying and tedious. Unless the author asks for an opinion assume they know what they’re doing. Same goes for review “comments,” that interrogate the author. “Why did you choose to use a for loop here?” is such a waste of time. I agree, stay within stylistic guidelines and lacking any try to use the same style as the file being edited. And just focus on the code itself: does it meet the specifications, is it clear that it does what it’s supposed to, are all the T’s crossed and I’s dotted? Good, go. Brevity is it’s own form of clarity when used well. Update ... and remember to leave your ego at the door! Talk about the code like an object of study.

tapanjk 2y ago

One of the tendencies I see in reviews is that the reviewer focuses on the wrong stuff. For e.g. when reviewing the design document, they focus on the validity of the requirements and they may even stray into commenting on formatting and grammar in the document; when reviewing code, they focus on design; when reviewing test cases, again they focus on reviewing requirements; when reviewing deployment scripts, they focus on the choise of infrastructure. Basically, people tend to pay attention to things that are already past the decision stage while ignoring the matter at hand.