About the author
Albert Row is a senior software engineer from the San Francisco Bay Area with over 13 years of experience as an individual contributor, technical lead, architect, open-source contributor, and manager.
Albert has been a certified reviewer on PullRequest since December 2019.
So… Why Do We Do Code Reviews?
We do code reviews to improve the structure of changes to software, reduce the number of regressions entering the systems we’re responsible for, to share relevant knowledge, to learn as reviewers, and to stay up to date on changes entering the system. Did you see “to engage in fights over formatting” in the previous list? Nope. But do they happen often? Yes.
Code Style Is Important, but It’s Table Stakes
Having a readable format for code in your repository will make it easier to maintain and easier to review. Using human reviewers to verify code style wastes a lot of valuable time, however. Given the difficulty of reviewing and maintaining inconsistently formatted code and the painstaking nature of checking every line for correct code style, establishing a style guide becomes table stakes for productive development and review processes once there are more than a small handful of developers on a project. Having alignment on what that format should be in your team is a good idea. But no human is perfectly consistent - and thus relying on humans to enforce good code style will never be perfectly consistent.
When Conflict Arises From Code Review
Typically conflict arises on reviews when offense is given or taken in one of two ways: either someone’s core beliefs about how good software is created are being challenged or the feedback given feels petty. Things like style nits are the type of petty feedback that can make experienced software engineers want to flip a table when they’re coming from a colleague. Computers, on the other hand, can be impartial mirrors in all of this. That’s why it’s best to automate the style guide and let computers make all the obnoxious nits!
Automating Code Style Checks
All major languages have libraries for enforcing code style. They are generally referred to as linters. Ruby has Rubocop, Python has a plethora of tools, some of the most used being Flake8, Black, isort, and pylint. Golang made a good decision early on and created a language-standard formatter that’s built into the compiler - if only all languages were so enlightened!
The best way to automate conformance to a style guide is to add a code check to the CI process. When the build fails when the style guide is not followed, developers will conform quite quickly to the expected style.
How to Avoid Major Refactors
When introducing a linter to the CI process on an established project there will be a strong temptation to go through and fix every single failure on day one. Many tools have systems that automatically fix lint failures, and simply running that tool and walking away sounds attractive. Unfortunately, as with so many things in software development, no good thing comes for free. Typically these large refactors introduce bugs to the software, thus reducing quality rather than increasing it. For that reason I find it’s best to introduce style checks slowly.
Most linters have a way to disable checks for certain rules or certain files. I prefer to disable checks for certain rules when they’re not passing, and slowly turn those rules on for subsequent pull request reviews, fixing the related failures for one rule at a time. This will allow you to create smaller pull requests that are easier for you and your teammates to review and test.
Where to Run Automated Code Checks
There are typically three options for where to run linting checks:
- As a dedicated command separate from the test suite.
- As part of the test suite.
- In a precommit hook in source control.
Each approach has its adherents and detractors - developers are an opinionated bunch and this is certainly an area where disagreement abounds. My recommendation is to keep the linter as a separate command and not to bother with precommit hooks. My best explanation of my preference is that creating commits should be encouraged, and especially if you squash commits on merge there’s no real need for every single commit to pass the test suite. Others have strong differing opinions here, so please do what your team feels would be best. I don’t think there’s truly a wrong answer.
How to Handle Disabled Checks in Old Code
On occasion you’ll encounter files that are so brutally old and crufty that the best solution is to turn off linting on that file and walk away. I recommend looking at how frequently these files are modified. If they are under active development, congratulations! You’ve found some gnarly and painful technical debt that should probably be tackled. If they’re infrequently changed and the related code is running fine, it’s probably best to leave them well enough alone.
How to Handle Disabled Checks in New Code
Pull requests will occasionally occur where the author has hit an intolerable frustration level with the linter and put in a comment disabling a certain rule for a line or two - doing so is usually fine, especially if it’s hindering progress. At other times, the author will have hit a table-flipping frustration level with the linter and disabled linting on massive sections of code. This is not OK and it’s time to figure out why the author felt the need to step out of bounds. It may be best to approach this gently and from a place of understanding and compassion, given the author’s likely fragile mental state.
Evolving Code Style Over Time
Unless you’re working in a language with a built-in formatter (Go, you’re making us all look bad…) you will want to evolve the style of your software over time. The best advice I can give here is to get buy-in from as many developers on the team as possible before making the change, make it very obvious the change is coming, document that the change was agreed to by everyone, and then introduce the change at an agreed-on time in an agreed-on code review.
The hardest part of changing the coding style guide is people, not the technology.
TL;DR
Adding an automated code style check to your CI process will save time and frustration on any software development team over time. Introducing one is a good idea! Make sure to bring in rules slowly to existing projects to avoid major refactors and gain buy-in from the team on the selected style rules for your projects.
Image by Marco Verch Professional Photographer
Also be sure to check out:
