ACAB: Fire the (code style) cop in your head

Photo by Karsten Winegeart on Unsplash

Let's get something out of the way right at the start: I hate enforced automated code style formatters. This post started because I had an axe to grind, and I made the ill-advised decision to haul my grindstone out into the public square. Multiple times. Over weeks.

Disclaimer! I primarily write Kotlin code that I build with Gradle, in Intellij IDEA (in order of importance). Since many of my complaints are about specific implementation-level issues with code style formatting tools, if you don't also build Kotlin with Gradle, don't worry about it! I'm just an internet crank and you are relieved from the duty of telling me how wrong I am.

Second disclaimer! Enforced code style is not the same thing as linting for usage issues. I like linters! Please tell me I forgot to close a stream, or am calling a Java API with ambiguous nullability.

Ok, let's gooooo.

Arguments in favor of enforcement of consistent code style

Consistent code style is better than inconsistent code style

Sure. But what do we mean by better? The general claims are:

1. More readable
2. More maintainable
3. Aids with achieving flow
4. Contains fewer errors

I think we've already run into an issue, which is we need to clarify what "style" means. Are we just talking about where we break lines and how long those lines are? Or do we mean something deeper? If we are talking about line breaks, I think the argument is pretty weak. Sure, all else being equal, if two pieces of source code follow the same general style, I will have an easier time with pattern-matching and "understanding" them. But what if my pattern-matching misleads me? What if I gloss over the fact that two structurally-identical blocks contain a single semantic difference that entirely changes the behavior? Maybe that single difference is easier to spot if the whitespace is identical, or maybe it's easier to miss because my brain too-readily dismisses the second block as identical to the first. This is actually what comprehensive tests are for. If this is the kind of error you want to catch, you actually just want tests.

What if we mean "style" in the deeper sense? I had a colleague once who routinely pumped out massive amounts of code, all formatted the same as older code near it, but whose style was so different that loading it into memory (that is, reading and understanding it) was a huge effort.¹ The naming conventions were different; there was very heavy use of Kotlin scoping functions like run and let; and lazy callbacks, generic extension functions, lambdas and lambdas-with-receivers were simply everywhere. (None of that is to say there was anything "wrong" with it. It was just different from all the other code nearby.) Automated style enforcement failed to achieve any of the above-listed goals regarding this code, although I suppose one could argue they prevented comprehensibility from being somehow even worse.

Ok, so, automated style formatters can't possibly help with "code style" in this second sense. We can dismiss it from further consideration but we also must acknowledge that our goals with automated style enforcement may be entirely defeated by orthogonal, essentially social, concerns.

Another argument I've seen in favor of consistent code style is that it can help spot code smells, or usages of known-problematic APIs, such as Java's ObjectOutputStream.² I would say that if this is something you care about, then what you really want is a linter. As I said above, linters and code style formatters are different things. We can forbid ObjectOutputStreams without going anywhere near the question of consistent code style enforcement.

To conclude this section, I will say that, all else being equal, consistent code style is Good™️. Its goals, however, are easily defeated by orthogonal issues, or actually achieved with other kinds of tools (tests, linters) that are themselves completely orthogonal to code style per se.³

Arguments against enforcement of consistent code style

To reiterate part of the conclusion above, in some cases I think we are simply making a category error. We don't want consistent code style, we want more and better tests, and stricter linting for issues that can't be caught by the compiler.

Therefore, my primary argument against enforcing code style is that its proponents haven't made a strong enough case. In a world where managers are telling me to do "less with less," and there have been mass layoffs and productivity concerns and increasingly strict requirements that all work be explicitly tied to identified business needs, why am I spending any time at all on whitespace-normalization tools?

I want peer-reviewed, high-quality research showing that enforcing consistent code style helps with business outcomes, or I would like to never talk about this again. There is a massive opportunity cost here that is simply not being acknowledged.

Most of the rest of my arguments are around how painful it has proven to use the available tools. That is, they're implementation-level issues that could, in principle, be resolved. But I will simply point to the above paragraph and ask why I should spend any time on this.

For a code formatter to even be considered usable, it has to be able to autoremediate all issues it can report. If it can report issues—as errors even—then it must also be able to fix them. Yet, not all code formatting tools can do this. What's worse, they may change their rules in minor or patch releases, breaking automated dependency updates because they'll require a human to manually add line breaks to dozens or hundreds of source files. This might be a bug or it might be a design limitation. Either way, we can simply dismiss these immediately from consideration. (Bearing in mind that we won't be fixing them, because who has time for that?)

Now, what remains are the class of tools that can autoremediate all issues. This is great! Maybe we can just skip this argument about enforcing code style and just slap this tool onto our project and away we go! But... when should we run these tools? Do we run them only in CI to avoid adding friction to the inner dev loop? This would certainly lead to an explosion of commits like "appeasing code style cop" and longer time-to-merge. Can we run them locally instead? Ok, when? Here are some options:

1. Pre-compile. Using Gradle, simply add a configuration block like this:⁴
   

   tasks.named("kotlinCompile") { t ->
     t.dependsOn("fixStyle")
   }
   

   But that's not great, because if there's a compilation error, then the fixStyle task will fail with its own opaque parsing error, instead of the more expected compilation failure. Ok, what about...

2. Post-compile. Using Gradle, simply add a configuration block like this:
   

   tasks.named("kotlinCompile") { t ->
     t.finalizedBy("fixStyle")
   }
   

   Turns out that's not great either. The fixStyle whitespace-normalizer will rewrite your source code after it's been compiled, meaning there is now a mismatch between the compiled class files and the source files. Debugging breakpoints can move around, and if you are indeed in a debugging session and commenting/uncommenting blocks of code, your style formatter will treat the block comment differently from a source block, such that when you later uncomment it, it may not be syntactically correct, requiring an additional step in this innermost part of your dev loop. And since you're debugging, that suggests your focus is on fixing bugs, and now you have to contend with the annoying friction of this tool you already don't like breaking your code.

   Both of these approachs have another issue, which is they make your builds slower. A well-configured tool will at least only run on changed files, but nevertheless it is "wasting time" when you just want to run your code.

3. Pre-commit. Add a git pre-commit hook that runs the tool before you can commit. This approach is better, but you have to make a choice:

   a. Pre-commit with Gradle task. This is going to make your git commit much slower, because Gradle is slow.⁵ That's annoying.

   b. Pre-commit with CLI tool. This can be much faster, since you won't have to deal with the Gradle configuration overhead, but now you have to be very careful to ensure that the CLI tool and the Gradle task do the same thing, else you might end up with mysterious CI failures that frustrate your developers. So be careful.

   In both these cases, you might run into issues if you use git add -p to make fine-grained commits. There are ways to resolve this, but it makes the tooling more complex to maintain. And of course, git is usually very fast, so making it slower can be frustrating. But, in some tests my team ran, the cost was on the order of ~2 seconds. Not great, but tolerable I guess, especially if it lets us move on from this topic forever.

4. Pre-push. Add a git pre-push hook that runs the tool before you can push.

   We have similar concerns to 3a and 3b above, so I won't repeat them. The biggest difference here is it resolves some issues with pre-commit, but also will require developers to add a new commit, orthogonal to their main concern, that is basically git commit -am "Appease code formatter." Not the worst thing in the world.

Grinding my axe

As I said at top, this post is really about grinding an axe in public. So now I want to share some of my least-favorite formatting decisions. I truly do not understand how any of these can be considered optimal for readability.

before

@get:PathSensitive(PathSensitivity.RELATIVE)
@get:InputFiles
public abstract val buildFiles: SetProperty<File>

after

@get:PathSensitive(PathSensitivity.RELATIVE) @get:InputFiles public abstract val buildFiles: SetProperty<File>

Wtf? The code formatter, rather than treating the max line length as a maximum, appears to be treating it as an ideal. If the property plus all of its annotations can fit on one line, it does that! Otherwise, it leaves the declaration untouched.

before

testCases().flatMap { testCase ->
  gradleVersions().map { gradleVersion ->
    arrayOf(testCase, gradleVersion)
  }
}

after

testCases().flatMap { testCase -> gradleVersions().map { gradleVersion -> arrayOf(testCase, gradleVersion) } }

Same problem with treating the max line length like an ideal to be achieved, with the additional wrinkle that now it's collapsing multiple scopes onto a single line, making it harder to visually match curly brace pairs.

before

if (
  veryLongFunctionThatReturnsABooleanYeeeeeaahhhhhhhhhhhhhhhh()
  || veryLongFunctionThatReturnsABooleanYeeeeeaahhhhhhhhhhhhhhhh()
) {
  // do something
}

after

if (
  veryLongFunctionThatReturnsABooleanYeeeeeaahhhhhhhhhhhhhhhh() ||
    veryLongFunctionThatReturnsABooleanYeeeeeaahhhhhhhhhhhhhhhh()
) {
  // do something
}

This one really annoys me. In the before-case, while I'm iterating on my branching logic, I can easily comment-out individual lines and the code remains syntactically correct. In the after-case, I simply cannot. It takes more keypresses or even mouse movement where before it was a single keypress. I like to minimize mouse movement to avoid repetitive strain injuries.

Axe-grinding in the community

In the Mastodon thread linked at top, some math-centric devs posted examples of auto-formatting that made their code less readable in their view, when they have data that is best represented as a table. I typed their examples directly into my editor and ran my team's tool on it—it produced an even worse result than the screenshot!

before

val v = (data[pos++].toLong() and 0xffL shl 56
  or (data[pos++].toLong() and 0xffL shl 48)
  or (data[pos++].toLong() and 0xffL shl 40)
  or (data[pos++].toLong() and 0xffL shl 32)
  or (data[pos++].toLong() and 0xffL shl 24)
  or (data[pos++].toLong() and 0xffL shl 16)
  or (data[pos++].toLong() and 0xffL shl 8)
  or (data[pos++].toLong() and 0xffL))

after

val v =
  (data[pos++].toLong() and
    0xffL shl
    56 or
    (data[pos++].toLong() and 0xffL shl 48) or
    (data[pos++].toLong() and 0xffL shl 40) or
    (data[pos++].toLong() and 0xffL shl 32) or
    (data[pos++].toLong() and 0xffL shl 24) or
    (data[pos++].toLong() and 0xffL shl 16) or
    (data[pos++].toLong() and 0xffL shl 8) or
    (data[pos++].toLong() and 0xffL))

Wat. Among other weird problems, it exhibits the same behavior as my complex boolean above: it moves the or from a prefix-position to a postfix-position.

Someone else suggested adding 0 or at the front... and yes, that plus an additional pair of parenthese did help (note, I am not personally very good at bit operations so I'm not sure if I didn't just break this code!).

before

val v =
  (0L
    or (data[pos++].toLong() and 0xffL shl 56)
    or (data[pos++].toLong() and 0xffL shl 48)
    or (data[pos++].toLong() and 0xffL shl 40)
    or (data[pos++].toLong() and 0xffL shl 32)
    or (data[pos++].toLong() and 0xffL shl 24)
    or (data[pos++].toLong() and 0xffL shl 16)
    or (data[pos++].toLong() and 0xffL shl 8)
    or (data[pos++].toLong() and 0xffL))

after

val v =
  (0L or
    (data[pos++].toLong() and 0xffL shl 56) or
    (data[pos++].toLong() and 0xffL shl 48) or
    (data[pos++].toLong() and 0xffL shl 40) or
    (data[pos++].toLong() and 0xffL shl 32) or
    (data[pos++].toLong() and 0xffL shl 24) or
    (data[pos++].toLong() and 0xffL shl 16) or
    (data[pos++].toLong() and 0xffL shl 8) or
    (data[pos++].toLong() and 0xffL))

This is, admittedly, better. However, it's still moving the or over to the end, which I just do not understand. And maybe more to the point, I've just had to adjust my behavior to appease a tool I don't like in the first place. It's producing uglier code that's harder to read and maintain, and now it's also making me dance! Badly! Not a fan!

What about selectively switching formatting off and on? Maybe we just wrap our tabular data in //stylecop:off and //stylecop:on. Boom, problem solved. Well, the fact that this is an option means that we cannot just slap this tool onto our codebase and never talk about whitespace again. Now we have to talk about it in every damn PR where these options appear, or could appear in the eye of the reviewer.

Summing up

I put off writing this post for a long time because, as I said, I find this whole subject a waste of time in the face of competing priorities. My hope is that, having written it, I will never have to write about it again—I'll just link back to this post. Thanks for reading! Looking forward to not reading your contrary arguments ❤️

Endnotes

¹ He used to work at Google where, the rumors say, developers are rewarded for writing complex code. up

² Described as "the JVM equivalent of contracting a minor illness". up

³ See also Research into Advantages of Having a Standard Coding Style up

⁴ There is no task (that I'm aware of) named fixStyle. I have invented it for this post to avoid getting into fights with specific tool makers, who are working on truly difficult problems (that I simply don't care about). up

⁵ I am obligated to say that it's Gradle's single-threaded configuration phase that is slow and annoying. Task execution will be basically identical to running the CLI tool. Configuration caching and the promised land of isolated projects may yet save us. up

Comments

[Cody Swartz]

Well someone has to leave a comment not to be read here 😆

Damnit I wanted to not write more, but this story came to mind.

I had a coworker that loved to call out adding extra white space after certain block types within a function during peer review. It wasn't really anything consistent in our code base, just something he wanted. I would joke that if it's that important he could write a linter rule with auto fix for formatting. It just drove me nuts that we would waste iterations of CI and our time on something so meaningless and trivial; this would be marked as a blocker on the reviews. It was easier to just fix it up and move along..

PS: that seems nuts to be adding useless code for the sake of auto formatting. There have really been some conventions through the years that have irked me such as the "cliff hanging" that folks notoriously utilized in Ruby. I believe it finally died off, but I'm sure it's still out there..

I hate to hop in a codebase that has a formatter configuration and the code hasn't even been utilizing it, and the defaults it has are ugly from who knows what template that hasn't aged well. If you run it on the whole code base it might as well be an uglifier 😆

[Stephen Johnston]

Look up "disagree and commit." Code formatting for teams live right in that style of management. Get an automatic formatter. Decide what it will enforce for your team. Start with the style of a library or project the team lead likes. If there is a debate on a specific item then spend five minutes on it and flip a coin if no one concedes. Disagree and commit, then reevaluate after 3 months of usage, then 6 months, then 1 year. Most of all, stop acting like just because it is hard to "prove" that consistency is valuable, that has less value then your stubborn refusal to adopt space over tabs (or whatever dumb code style bike shed item) or vice versa. Consistency has proven valuable in every other human endeavor. It has value in software.