This article was originally published in the 37signals dev blog.


Nitpicking in pull requests reviews means offering insight that looks excessive, pedantic, and unimportant. It’s a pejorative term. I don’t question there are cases where the term applies, but many folks assimilate it to paying attention to the small stuff. By projecting all these negative connotations, this practice suddenly becomes undesirable: mind the big picture or leave the pull request alone. With that, we disagree. Small stuff when writing code matters, a lot.

Naming things is a usual suspect when illustrating nitpicking. It’s also the example I have the hardest time swallowing. With the importance of names to make code understandable, how can anyone consider discussing those superfluous?

Check what Jeffrey Hardy told me in this recent pull request:



Or what I told Matt Hutchinson in another one:

Great code aesthetics is a distinctive trait of Ruby, and one of the Rails pillars. Compare:

In Javascript:

if (this.hasParamTarget) { this.paramTarget.value = id }

In Ruby:

param_target.value = id if has_param_target?

In Python:

datetime.now() - timedelta(days=5)

In Rails:

5.days.ago

Does this attention to aesthetics make you a happier programmer? And this is not a rhetorical question. Tons of outstanding programmers couldn’t care less about this stuff. But if you do, paying attention to code style and aesthetics in pull request is logical and desirable. This includes suggesting code that could be formatted more cleanly, minor logic simplifications, or slightly more idiomatic alternatives. Precisely the kind of stuff that is often labeled as nitpicking.

Now, check these examples:

Jeff points out a conditional that could be formatted more cleanly:

And, in the same review, he suggests a more idiomatic way of using enum:



Here, I suggest Jeff to use .ids instead of pluck(:id). I learned about this one in another pull request review by Lewis Buckley back in the day. Nitpicking is transitive.



In this other example, David suggests me some minor simplification of logic:



Here, Jeff suggests me to get rid of a guard clause:



And I suggest something similar to Matt here:

Consistency helps to make code easier to understand and, thus, more maintainable. In other words, it’s priceless. Consistency is elusive, though, and very hard to achieve in large codebases as time passes and different people work on them. By paying attention to the big picture you may achieve consistency at the architectural level, and by using linters, you can expect consistency at the code-formatting level. But what about the very thick middle ground? This is where coding standards live.

Thorough pull requests that pay attention to the minutia are essential to enforce coding standards and transmit them through a team. For sure that documentation can help, and that nothing replaces skills and experience, but I can’t imagine how a codebase can keep a high level of consistency unless eyes very familiar with the in-house style review changes with a magnifier.

A good example is the preference to avoid guard clauses unless they save you from a long sequence ahead – as showed in the previous section. Whether you agree with a guideline or not, the value of conventions resides in having them in the first place. They help with consistency; and the most interesting and juicy ones can’t be validated automatically.

To consider the small detail in code as unimportant and even pedantic reminds me of the vilified architect role in our industry: the one that doesn’t really code, just cares about architectural concerns and patterns, the stuff that matters. The problems with this role are apparent to most but, at the same time, many teams consider nitpicking something to avoid.

The big picture matters, for sure, but computers don’t execute big pictures, they execute lines of code, so how you compose those matters too. And code-aesthetics lover or not, everyone benefits from consistency.

I just had to check a few recent pull requests I was involved in to find many examples to pick from. Isolated and out of context, any of those suggestions is objectively trivial. But their aggregation is not. This approach is part of our development culture. This is how new people learn the in-house coding style, how we keep a high-quality bar in our code, and how we favor consistency.

I have ended up assimilating nitpicking in code reviews with being meticulous, thorough, and rigorous. I encourage you to review the threshold beyond which you consider some review advice as nitpicking. Maybe it is not aligned with what you value as a programmer.

---
jorgemanrubia.com