The Case for Nitpicking

Why you should discuss style in code reviews

It somehow has become common wisdom that you should never comment on style on code reviews. It is supposedly toxic, as everyone has their own taste, and no style is better than any other. My purpose with this essay is to destroy that notion. Specifically, I argue that:

• Some code styles are better than others

• Some people have better taste in code than others

• Preocupation with code style can be a cornerstone of a high-quality engineering culture

• You should be nitpicky on code reviews

Programmer happy for receiving too many comments on their PR. Generated my Google Gemini.

Isn't it all a matter of taste?

What people mean when they say that something is a matter of taste is that there is no objective standard to measure it. For example, preferences in music are a matter of taste since we can’t quantify precisely what’s the better song.

The weak form of this notion is obviously true: it’s hard to argue definitively whether Eleanor Rigby or Strawberry Fields Forever is the better Beatles song¹. The problem is that people take that fact as evidence for the strong form: no music is better than any other. But that is clearly false. Mozart or Pink Floyd is better than my arhythmic drumming on my desk².

Good taste is a real thing

Practitioners in any field organically develop a collective notion of quality. Though never fully objective, they can usually agree on principles of what makes a good piece of work. For example, a movie whose plot is well connected with each action moving the story forward is better than one where random events happen one after another³.

In engineering disciplines, the notion of quality tends to be more objective. A bridge that supports more load with the same amount of concrete is simply better. Software Engineering sits somewhere in the middle, but closer to civil engineering than music. Competent programmers can usually explain what makes some code better than another. They have a whole vocabulary just for that, with terms like readability, testability, consistency, and cognitive load.

There is still some level of subjectivity when applying these principles though, and especially tradeoffs. A more testable code can often be less readable. Competent programmers can disagree about what is the best option on concrete cases, but the arguing between them is healthy for all, leading to each becoming more skilled and the code becoming better at the end.

Code style matters

I concede some matters of style are irrelevant, for example whether tabs or spaces are better. More often however, theres’s plausible argument for one option over another.

For example, compare these code snippets:

if (condition)
  action();

if (condition) {
  action();
}

The second form has a concrete advantage in that it prevents bugs where statements that should be in the if body are left outside it. This case is much less subjective than tabs vs spaces, and I know of no style guide that mandates the first form, while many mandate the latter.

There is a spectrum of subjectivity, with tabs vs spaces on one extreme, brackets on single line if bodies in the middle, and dependency injection for testability in the other. When a company adopts a culture of “we don’t discuss style here” they are saving time on tabs vs space, but they are also getting less testable code.

I argue that “we often argue style here” is the superior principle. There are just better and worse ways to do it.

Why discuss style

Some people who defend that you should never discuss style in code reviews say that all style rules should be applied by linters instead. But where do linter rules come from? They are invented by competent programmers arguing style.

When you say we should just defer to the linter, you are implicitly saying some people know what the fuck is going on, but we are not among them. While that might be true for some people at some companies, I would never want to work at such a place. Would you?

In other words, programmers create knowledge of what is good code through reflection, reasoning and arguing, which often happens in code reviews. Such arguing should be encouraged rather than chastised⁴. It signals we are the kind of place where we discuss what the best solution is, a place of ideas, collaboration, and learning, instead of a place where we “shut-up and dribble”.

Discussing style can be a catalyst and marker of a broader “culture of criticism”. We not only argue the best way to test code, but also the CEO’s product proposal. We will not blindly execute it, but instead take it as an idea to be discussed and improved upon. The CEO will like us better for it, since we will be their intellectual equals contributing to the vision rather than mere code-monkeys.

I find this Zen Buddhist quote encapsulates it well:

The way we do anything is the way we do everything

The wrong way to be nitpicky

The reason a lot of people have an aversion do discussing style is that they have had unpleasant experiences with authoritarian assholes. But that is no good reason to not be nitpicky. Instead, we just need to do it in a non-authoritarian, non-assholery way.

By precisely describing such assholery, we can avoid the toxic behaviors and get the benefits of a culture of criticism without the bullshit.

Authoritarian assholes don’t treat others as equals

If I offer an idea of how your code could be better, I expect you to take the idea critically and adopt it if it makes sense to you, or counter argue. If on the other hand I don’t consider you my equal, I’ll give you an order to change the code.

Authoritarian assholes don’t explain themselves

Good ideas usually take explanations to allow criticism. I can only disagree constructively on tabs vs spaces, if I know why you are arguing for tabs.

Authoritarian assholes don’t accept counterarguments

If I give a valid counter argument and it’s dismissed without consideration, we are not having a debate. Instead, you are just bossing me.

Authoritarian assholes are not polite

Humans have emotions, so the same statement can be taken as a helpful suggestion or a personal attack depending on tone. For that reason, we must be extremely polite when suggesting improvements. Contrast two forms for the same code review suggestion:

Your code has horrible performance for large inputs. Why did you not use binary search?

I think binary search would have better performance here, since our datasets can be quite large. What do you think? 😊

The right way to be nitpicky

Cultivate an environment of learning and criticism

By justifying your suggestions, being polite and trying to teach rather than to command, your comments will be taken in a positive way, and you will build a reputation for quality and competence.

Build a style guide

People have a good point that excessive comments can slow down code review. The fix is to collaboratively build a style guide:

1. Proposals to change the guide become the default space to discuss best practices. Naturally, recurring code comments will graduate to style guide rules.

2. The guide decreases the burden of code review on both sides. The reviewer can simply link to a rule, and code authors can learn best practices by reading the guide. That way they will write better code to begin with, rather than changing it in review.

3. The guide makes code review more impersonal. I’m not picking on you; I’m just applying the guide.

I recommend structuring the style guide rules in this format:

Rule stated imperatively

Small paragraph giving more detail.

Rationale: Why we think it’s good to follow this rule.

The imperative title and the rationale are the key parts. The first makes sure the rule is crisp and easy to understand and follow. The second makes it open to criticism. We don’t follow it just because.

To illustrate the format, here’s a real example from my company's style guide:

Avoid Null Words in Identifiers

Null words are words add almost no meaning to identifiers. Here are some example words which are often null:

• Data

• Info

• Metadata

• Type

• Util or utils: it usually means a set of unrelated functionality is being bundled for no good reason. It's better to have several small, targeted modules than a large incohesive one.

In some cases, words like those might still make sense, but they should be used judiciously, only when they highlight an important distinction.

Rationale: Null words make identifiers longer and make code confusing when different developers use different null words for same concept. For example, is UserData the same as UserInfo?

Furthermore, the distinction between the "thing" concept and the "information about the thing" concept is usually harmful to code clarity because it can be applied to all symbols. For example, to apply it consistently one would need to add "Info" to all class names. Ultimately, all abstractions are "about the thing" rather than the "thing proper".

Automate the style guide as much as possible

Every style rule that could be automated should be⁵, as it saves effort and makes problems be caught earlier when they are cheaper to fix. The automation also becomes a learning tool. You don’t even need to read the guide, it will be taught to you incrementally as you run the linter.

The right way to take suggestions

When you first join an environment like the one I’m describing, it all might seem just too much, but you will become a better engineer if you learn to thrive in it. Here’s some advice to make it easier:

• Be curious. If not explained, ask what makes a suggestion better. Not to reject it, but to learn.

• Assume good intent⁶. There is often ambiguity in communication, so try to resolve it in a positive direction as much as possible. Being adversarial or overly sensitive personalizes issues and detracts from a culture of criticism.

• Recognize good taste. Some people are right more often. Learn who those people are and be especially attentive to their suggestions. They usually also have the best explanations for their feedback.

In conclusion

Contrary to popular wisdom, an environment where people are nitpicky with each other can be productive and a pleasure to work in. We just need a respectful culture of criticism and learning. That culture will not only be the best in technical terms but will lead to Software Engineers becoming true problem solvers rather than code monkeys. That’s what I intend to be. What about you?

1. One could write an entire article defending each one. Eleanor Rigby has better storytelling, while Strawberry Fields is more poetic. Strawberry Fields is more experimental, while Eleonor Rigby is easier to appreciate.

2. Taking the true weak form of an argument as evidence for the false strong form is known as the Motte and Bailey fallacy. It’s perhaps the most common defense of falsities in the modern world, so you should always be attentive to it.

3. There will always be exceptions, but they prove rather than negate the rule. For example, The Big Lebowski is a widely acclaimed movie, despite the fact the plot is driven by a series of misunderstandings and coincidences rather than a tightly woven causal chain of events. In general, it takes a master to subvert a rule of the field and you can tell the rule breaking was a stylistic choice rather than simply being lack of skill.

4. All virtues in excess become vices. This is called the “Golden Mean” and has been known at least since Aristotle. Excessive, or more precisely unjustified, nitpicking is bike-shedding, which should also be avoided.

5. I suspect that with LLMs we can now automate even high-level style guides. You should be able to create a bot that uses the guide as a LLM prompt and proposes code review comments on a PR. Unfortunately, I have not seen that idea implemented yet. If you have, or want to work on it, drop a comment on this essay.

6. Another formulation of the same concept I like is Postel’s Law: Be conservative in what you do, be liberal in what you accept from others.