Nitpicks are (sometimes) okay!
When writing code, some things are obvious - don’t commit your passwords and tokens.
Some can be understood with a bit of explanation - write IaC instead of manual provisioning for reproducibility.
But, some never really made sense to me when I was starting out. Why does my commit need to follow a specific pattern? Why does it matter whether I use semicolons in JavaScript? Why do I need to add the word “TODO” at the beginning of a tech debt comment?
Nitpicks are trivial review comments that seem opinionated, won’t break anything right now, and you can probably live with. It’s your choice whether you want to accept the suggestion.
Whenever I got a nitpick I didn’t understand, I asked why. Sometimes, it turned out they didn’t either - it was just de facto for them. Most of these are, in fact, just opinionated ways of writing code. Your application will still run, and nothing would break.
But, there are some slighter nuances that I learnt along the way. The more code I wrote, the more I started understanding how some of these nitpicks (albeit silently) help. So, the next time someone gives you a nitpicky review comment - ask them why and learn how it can help.
Before we begin, a disclaimer - this isn’t an excuse to comment on every single line of code! Everyone writes code differently, and unless you have a reason, and your team has agreed to it, the nitpick isn’t valid.
Aggregation, automation, search#
Codebases grow larger with time, and if you work with at least a moderately large codebase, you can’t keep everything in the back of your head.
When scrolling through some code, I realized there were a lot of tiny debts which we forgot about scattered throughout the code.
In an attempt to pull them together into a ticket, I opened up my IDE, quickly searched for // TODO: and extracted it out in a jiffy!
For an embarrassingly brief moment, I was overjoyed with the productivity and ready to create that card for the team! That was short-lived, however, when I realised this was only how I wrote TODOs. There were dozens of other debts, //todo, // to do and some without any prefix at all lying somewhere in a dense jungle of comments.
Standards are important in a codebase, it helps establish a contract within your team on how you all agree to write code.
When a time critical bug comes in and you need to sift through your commits, a standard and elaborate commit message pattern helps navigate them with ease.
When you want to find something globally, having a standard can help you easily figure out the regex to search for.
When writing scripts to automate some changes across your entire codebase, everyone writing code a certain way means less edge cases to handle.
While it might not matter what standard is followed, what’s important is that it’s the same throughout the codebase. A lot of the searches, automation or scripts that you, or your team, write in the future will be easier if these micro-standards are maintained.
Some of these can be automated with code formatters like prettier, but this might not always be the case. If you’re unsure, check the rest of the codebase and follow what exists, or if you’re the first, be the one to set it!
Reading, scrolling, comprehending#
When working on a feature and figuring out how to get things written, one can accumulate a lot of hidden context, and to one’s own eyes, the final code can seem concise & perfect!
I was trying to fix a minor typescript issue which had been ignored in a test file for a couple of years. After spending a good ten minutes trying to figure out what was wrong, I decided to take a step back and relook at the test. I made a mistake - I was going down rabbit holes with wrong assumptions. The test was titled should <some scenario> when, in fact, the test was meant to be should <some scenario> when <some condition>. The fix was easy to pin down once I realized that and figured out what values to default to.
A variable name with just enough context to give the next person an idea of your thought process helps save time for them, and takes just a few extra seconds for you.
Splitting a massive file into two smaller manageable files can help the next person navigate easier through the code.
Extracting good methods will allow others to skip reading an entire monolithic function, and quickly jump to the part they were trying to get to.
Everyone would have a different way of naming things, and frankly none of them are the absolute truth - if you see a nitpick on naming or “clean code” take a step back and think if the next person reading your code (who doesn’t have as much context as you just gained) will be able to understand it easily. If not, rewrite it!
Errors, security, performance#
“Handle all scenarios” is a loose term thrown around all the time. In all honesty, you can’t really handle all scenarios, what would you do if a meteor crashes on earth, should you create a backup on the moon?
However, a lot of tiny things like a null check, a semicolon, or not caching values between renders might look silly today but might be your downfall in the future. I’ll skip the anecdote for this section as I’m sure everyone’s been through something similar at some point.
A null check on a nullable variable you skip today because you know the value will always be there, is a production bug waiting to happen when someone changes some related code.
A skipped semicolon because of Javascript’s automatic semicolon insertion could prove fatal tomorrow if not careful.
A lot of these fixes can easily be incorporated with the right mix of Static Code Analyzers, Linting Rules, Code Formatters and Checks.
Code reviews should not be treated as gospel, but at the same time, nitpicks should not be disregarded outright - if you can reason it out, and you don’t overdo premature optimization, a tiny change can become a silent enabler down the line. Find the right balance between the two and you’ll have a codebase you are happy to work with!
Till the next post, またね!