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 :)
1 - time wide use a of a code formatter with an opinionated style setting to preempt such discussions is a huge productivity boon
2 - for "pattern" oriented changes that aren't easily checked in a linter, have an internal guide defining what you want in the codebase and then reference that. That again pushes the feedback back to the impersonal, and keeps it terse.
3 - leave the more substantial feedback for things like discussions around the business logic, or architecture of system, or implications of a particular change will have consequences outside the internal context of the subsystem
+1 on code formatting. It definitely takes care of a class of issues in practice. While I do have my own preferences, I think it's better to simply have something that can apply on save/paste and that can be the end of it. Changes to the code formatter/version and even swapping out a formatter can be relatively easy and still completely consistent.
I will state that I will prefer a formatter than can apply from a command line as well as integrated plugin. I don't like formatters that rely on an IDE only plugin, I've seen this with VS in particular.
> 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.
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."
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?
Not for no reason. I've got my IDE formatter set up {this way}. You have it set up {that way}.
We're working on a code base, and I need to work in a method... and my editor formats all the lines that I change. And now we're mixing up lines.
I've also seen devs do a "reformat file" each time they work in the file... which if there are conflicting formats reformats everything. If they didn't do that commit as a separate one, this makes reviewing changes much more difficult.
This is why the .editorconfig ( https://editorconfig.org ) that I check in to is at least 60 lines long with a lot of ij_java_ lines in there - so that we all use the same one.
I'm not their manager - I'm one of the senior developers on the team that gets the bulk of the code reviews to do. I am working on getting templates set up correctly in gitlab (and .editorconfig files in all the projects) so that new projects start out with correct formatting.
There has been a lot of "create the project, check it in, it needs to be deployed {Real Soon Now}" and it was never formatted correctly to begin with (with a mix of their own editor being tabs and copying and pasting from Stack Overflow having spaces - its very easy to see where they copied and pasted if I turn on show whitespace). Editorconfig doesn't reformat code that was pasted in and if they don't reformat it themselves, then the other style remains.
Yes, I do help them set up their editor correctly... though I will point out that most developers don't seem to care about formatting (or even editor warnings). In the past (current manager is different) I've had a lot of pressure to approve if it works because of time / business pressure which ingrained a significant amount of bad habits that didn't get rectified promptly.
So, again, this describes a scenario that can be addressed with tooling and management/communication.
The question I asked was why reviewers should flag code that correctly iterates over a list but does so in a different way than other code in the codebase. Are we straying into “foolish consistency” if we flag this, or is there a tangible benefit?
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.
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 :)