I'm Steve Barnegren, an iOS developer based in London. If you want to get in touch:
- I'm @SteveBarnegren on Twitter
- Find me on GitHub
- Contact me through my website
09 Feb 2022
Programmers can be a pedantic bunch. We like everything to be consistent. Uniform. Organised. Just so.
This mindset is as much a part of our culture and identity as it is good practice for our craft. Sometimes I wonder if developers come to somewhat view themselves as robots, running some internal software whose rules can be optimised and fine-tuned. With this mindset we strive for the human element of our practice to have the same traits as our software; it should be rule-based, consistent, and reproducible.
We love making the non-conforming conform, and there’s no more fertile ground than that of a code review. Here’s the thing though, what if conformity isn’t always worth the effort that we put in to it? What if we’re wasting our time on things that don’t make a difference to anything?
Every team has their ‘things’ that they’ve become perhaps a bit too obsessive over. Whitespace, braces, indentation, comment lengths, ternary statements vs if-else. Things that they might have once picked up on as being an indicator of code quality, but which they may be in danger of believing are the indicator of code quality.
I once worked with a developer who was really strict on identifying places in the code where we were using redundant self (using self.something when the self could be inferred), and each pull request was met with a flurry of suggestions to remove the redundant selfs. Seems reasonable, right? I thought that this was making things consistent, so I also took part in the war on redundant self.
Then that guy left the team, and we became a little less strict about it, and you know what happened?
Nothing.
Nothing caught fire. Nothing broke. Sure, there were a few more redundant selfs, and the team preference was loosely still to omit them, but they were harmless. There was one positive effect though: less noise in the code reviews, and more focus on the things that matter.
Some things in your code review really matter. For instance, a team member might spot a bug, suggest and architectural improvement, or identify some way to remove complexity.
The problem is that code review comments are often flooded with things like “Why not make this guard statement a single line”, or “We leave two whitespaces after a protocol declaration” or “You could use a ternary statement instead of an if-else” or “private comes before static in the method declaration”.
All of these things have the same thing in common. Once you make the change, the code does exactly 100% the same thing as it did before. It’s no more maintainable, no more scalable. You could recompile this, diff the binary, and see that it’s the same as before. So why do we bother?
Everyone’s at least once in their life lined up all the drinks cans in their fridge so that the labels all face the right way. I know you’ve done it. I also guess that you decided not to carry on this practice on account of it having no real value.
But there is some value. It’s aesthetically pleasing, and gives you a moment of satisfaction when you open the fridge. Perhaps it even allows you to identify the correct drink some small percentage faster. Now, if I were building a robot to fill the fridge then I’d probably make it line the drinks up all the same way, because why not?
If I, a human with limited time and energy, am filling the fridge, however, then I’ll skip the bit where I spin all the drinks around to face the same way, because the benefit is infinitesimally small compared to the time and effort that I expend in doing so.
We should see our code reviews like this. We have to weigh up if our energy is being well spent in fixing these ‘nitpicks’. If something really is just a nitpick, then well, probably not!
Sometimes it’s worth fixing something just so you can put your mind at ease and that nitpicky thing you care about will stop irking you. That’s ok, and is one of the best arguments for using tools such as linters; once all of the braces and spaces are uniform then you can just concentrate on the important things, right?
But we should recognise that it’s our own flaw to be irresistibly drawn to the insignificant minutiae of code formatting, rather than focussing on the details that contribute to the success of the code. Requesting a change to the code that results in exactly the same logic is the lowest value suggestion we could make. If the compiler doesn’t care, you have to question how much should you?
We have to be careful that we don’t come to believe that consistency of the minute details is what makes code good. What makes code good is scaleable architecture, useful abstractions, good design. No one really comes to regret writing a guard statement on a single line. At least, I’ve never heard that as being grounds for a rewrite, or sinking a once prosperous team.
The behaviour that we’re encouraging is the search for an ever increasing number of things to be pedantic about. We just add more and more things to the list. It starts with braces, then spaces, then naming conventions, then class layout, etc. Eventually you will probably yearn for the time that you could just get on with your job.
The worst type of suggestion is one from a reviewer who doesn’t actually know their own rules. They might like this guard statement on a single line, but that other one across multiple, but they can’t quite articulate why. In this case, the team can never learn or align on these rules, and this just results in ‘formatting churn’ for each pull request, that creates a drag on the team’s output.
New developers learn this ‘habit’ from their more experienced peers. Rather than considering the different trade offs between design patterns and approaches, they come to view surface level consistency and ‘neatness’ as being the goal that all teams should aspire too.
Once we come to believe that this is what makes code good, then we start actively search for new things to be pedantic about. More conditions to check in code review, more reasons to request changes to a pull request, believing that each of these rules represents some kind of mark of quality in our codebase.
In reality though, it doesn’t matter, and the team who just live in blissful ignorance of these ‘inconsistencies’ will probably put more energy into the things that do matter, such as what their code actually does.
Imagine that you could wear a pair of glasses that would allow you to see other people’s code with consistent formatting that aligned with all the nitpicky rules and conditions that you like, would you wear them?
I think it’s a no brainier. You would be able to focus just on what the code did, without being irked by the fact that the author used an if-else when you just ‘know’ that a ternary statement would look better here.
We can have these magic glasses! They’re just a mindset. We just need to be more chill about stuff. Here’s how we review an if-else condition:
We see that it’s an if-else, read what’s in the if condition, read what’s in the else condition, check the logic is sound, and just move on.
I’m not advocating for us not caring about things, but we should care about things proportional to the value that they provide. Shuffling some whitespace around is simply not worth the feedback cycle that you put in to it. It’s not worth the time it takes to make the comment. It’s not worth the time it takes for the author to fix it. It’s not worth clogging up the CI for a new build. You could have spent that time doing something of actual value.
Rather than nitpicking, we should instead focus on what the code actually does, not how well it aligns with a set of arbitrary habits that we’ve come to have Stockholm syndrome for.
So my advice is: when you’re about to make a nitpicky suggestion, take a deep breath, and just… let it go.
I'm Steve Barnegren, an iOS developer based in London. If you want to get in touch: