Code review “nits”
Imagine this: You just finished your first task at a new job, you know you just wrote some fine code solving a hairy problem in a very elegant way. This work makes you proud and you can’t wait to show it to your colleagues and start discussing what you missed, what they would do differently for reasons you did not foresee and if there’s any way to make it even better.
You double check the diff for the umpteenth time, double check the contributions guidelines yet again and with a mix of pride and anxiety click on Create Pull Request.
After a while your inbox starts beeping louder and louder and comments start pouring in. Breath in, click and… You missed a space. And please remove the last comma. And no new line at the end of the file. And we don’t indent code here. Also, please no nesting in SASS. And on and on and on.
This is way too familiar for far too many developers. And it usually comes from a very good place. Code styling is of the utmost importance to keep a codebase clean and consistent. We can’t stress enough how important it is to have one code style. Nobody wants to deal with double quotes and single quotes used interchangeably, nobody wants to deal with the cognitive dissonance of having to adapt to a new code styling for every new PR depending solely on which team member opened it.
As a result of adding more teammates and spreading the engineering team over 2 continents and 3 time zones, we started facing these issues recently.
Code styling couldn’t be learned by heart anymore and the overhead of on-boarding new users and having them adapt to the new rules by sheer force of will was becoming too big. People come with their own preferences, beliefs and opinions. Every unwritten rule is up for discussion and surely enough there. will. be. discussions.
We looked at ways to get rid of all kinds of nits and to agree on the rules we wanted to keep and those we wanted to add. Finally, we looked at tooling to make sure those rules are enforced without having to think about it.
Automating all this turns off our pattern-scanning mode hunting for styling issues, so we can focus on finding sneaky bugs, spot architectural issues and recognize when something is being solved in a particularly elegant way and when we just learned something we didn’t know.
Understanding other people’s code is the hardest thing, I would’ve done it differently is no good excuse to dismiss other solutions.
Ça va sans dire, Prettier is my favorite ever and I love it more than pizza.
I’m not a super opinionated person and the fact it comes with a very minimal set of options is a huge win. You install it and it just works.
We have it configured as a plugin for eslint.
It has a massive set of rules and it’s highly configurable, you tell eslint how you like your code and it will make sure that you stay happy.
Not only that, it will catch an entire class of bugs for you: unused variables, variables you forgot to declare, use of debugger in production…
We have it configured as a git pre-push hook to only check staged files.
Something that’s often overlooked is sorting your imports. We intuitively sort our imports by type, but that’s just not enough. Big projects end up with a huge list of imports (side note: this might be a code smell) and having to scan through them searching for something specific is taxing.
We’ve developed our own plugin (here’s how you can make your own) to sort and group imports our favorite way and to prevent an issue with styled components and the order we want our css files to be loaded.
Configured as a git pre-commit hook to only check staged files.
This will tell the editor how we want our code to look. Nothing worse than opening a new document and looking at that horrible 2 tabs indent when I want my 3 spaces indentation! Plus, nobody has time to check a new line is always there at the end of a file.
Nothing more to automate here, the editor will do all the work for you. You might need to install a plugin depending on which editor you use.
This is my least favorite but nonetheless a very important one. CSS is pretty straightforward and it might seem overkill to sort and organize your code, but that’s far from being the truth and a CSS file gone wild is pretty much an industry standard at this point. Csscomb, while outdated, makes your code easier to read and reduces the cognitive load of having to scan for your properties. While this might be controversial, the immediate win of seeing at a glance you declared the font rule twice is not. The reason this is my least favorite is very simple: we haven’t fully automated this yet. Not all our css code is “combed”, and it’s still something you need to remember to do.
Edit: after proof reading this post, Paulius went and opened a PR to fix this!
Here’s the git pre-commit hook:
We gladly eat our own food here, and we all enjoy when a PR contains a short screen recording video that goes through the code we’re about to look at and explains the controversial bits. I don’t need to be an asshat and point out we could have done it this other way if my colleague tells me they already tried that path and it didn’t work.
As an added benefit I find myself going back to the drawing board when I explain parts of my code and realize:
it doesn’t make any actual sense
I can name things better
I forgot to refactor it
When it makes sense, we also include a short demo of the feature/fix being reviewed. Never assume your reviewer is familiar with everything you’re working on. We tend to distribute PR reviews to various teammates to maximize knowledge sharing.
Have your IDE do most of the work
All these tools have their own IDE plugin. I’m a big fan of VS Code and these are the first ones I got from the marketplace (despite the name, they are all free) on a fresh install. Nothing more Zen than pasting a bit of ugly code and seeing the magic happen on save. So far the only plugin giving me some after thoughts is import-sort. From time to time having to shuffle lines around ends up scrolling the editor in weird places. Not as seamless as the integration of the other plugins.
Cover credits: Business vector created by freepik – www.freepik.com
Get expert workplace communication tips delivered straight to your inbox.
Written by Claudio Semeraro