An age-old story, engineers complaining about reviewing code, having their code reviewed, how long it takes, how much time out of their day it takes. No matter what, it is a vital part of our jobs, and isn’t really going anywhere fast. With the introduction of better linting, better CI, and better tools, it’s getting easier than ever. Responding to this, I wanted to take the first steps into improving our process, with the ultimate goal of reaching opinion only code reviews whilst simultaneously reducing cognitive load for engineers.
Today we look at the first step into improving this process, the reactive step.
This idea hinges on a pretty radical idea (at least in programming circles!) — code review should be entirely opinionated. That’s right, ain’t got no time for your facts here!
The grey area here is what we need to solve; “no-unused-vars” is a pretty standard rule, but there are lots of things, syntax, logic etc that is not explicitly covered. Though, as more and more people contribute to the various tools that we all rely on, this is going to get easier and easier.
I wrote an article on building linting rules — view it here — and I’m hoping that someone builds a lint rule for naming booleans, i.e variable name should begin with
is|has|should etc. (throw TypeScript into the mix and that should be fairly straight forward).
These rules are easy to find, easy to build, and easy to apply. Each time we add a new rule, we simplify the review process that little more. No longer do you have to look for unused variables in your code review because you trust the linter. So let’s look at that at scale…
How do we improve our code review
The first steps I took in making that grey area a little smaller was to do a review, of our code reviews. A code review, review. I made a list of all the things that we pick up in our reviews; This is an ongoing process and evolves over time as the code changes. If you’re doing this for the time, here are some tips to get you started:
- When you have a new engineer join your team, review their code with them and make a note of anything that sticks out that would get caught in your teams’ code reviews.
- Find recent pull/merge requests that have had a number of comments, go through them and make notes of the comments.
This will create a list of problems, some more common, some less, and this list is the foundation to start building on.
The Reactive way:
Someone raises a comment on a code review, the team generally agrees with the principle of the comment, a change is made to enforce that principle moving forward. Be that via a lint rule, a syntax guide change, a README change or something else, the more automated the better of course!
Over time, with the application of this principle, code reviews should get easier and importantly, quicker.
I would like to think, that every update to your
.eslintrc file should make your code review process a little better, taking an agile and long term approach to improving your code and your code review process.
Actioning those changes
A number of the smaller things can be picked up by linting, and some of them can be resolved by utilising the tools we currently use such as TypeScript, Prettier, testing etc.
One time someone left some code from an ES6 component in the file when upgrading it to TypeScript, so we added a rule, specifically for typescript files, that disallows the use of
import PropTypes from “prop-types" — very simple, and no longer would that code pass the CI.
Another example of this is; recently I noticed a comment on a code review around leaving an
only in a Jest test, a quick google and we find the Jest Eslint Plugin, and subsequently, our team no longer has to keep an eye out for tests with
skip. A small win, and one of that much larger picture.
Some miscellaneous challenges & solutions:
Spelling; in my current team we mostly use vscode, and cSpell to have an IDE spellcheck, there are others available for other IDE’s — you’d be surprised at how many times this has caught issues for us!
Typing; typing has changed so much about what we do for the better, not least of all the
type-check feature that essentially allows us to keep a very close eye on exactly what is happening and where.
Different rules for different files; Whilst this can get a bit complicated, utilising the “overrides” tools within your
.eslintrc file will help you manage very specific things. For example, we allow any number of lines in our test files…
These implementation has been “reactive” so far, rather than proactive, however, planning and foresight can improve that.
A lot of this work requires relatively constant effort, reviewing code, noting changes, putting tickets into our tech debt backlog to make positive changes to our CI process. Combining all of these things has improved our code so much, whilst freeing up engineers cognitive load to concentrate on the task at hand.
Moving forward with proactive changes
Naturally, the key to this is to be proactive, and spot areas with the potential for mistakes.
When planning improvements that can be made to testing (for example), there are certain parts of the codebase that need to be updated, this can lead to the inspiration of how to update your CI in order to enforce these things so they can be used when the time comes.