When I last looked for software engineering jobs, I came across a post that looked perfectly normal until I got to a line that said, “Participate in daily code inspections.” What the heck was a code inspection? It sounded pretty serious. And we’d be doing it every single day?
A quick Google search brought me to a Wikipedia page for “software inspection,”?which told me that “code inspection” was pioneered in the 70s at IBM. My grandfather’s cousin happened to work there at the time, so I asked him to weigh in. He said they were a “formalized effort by team members to go over each other’s code.”
But things have changed a lot since the 70s. And often, vintage terminology is a sign of other outdated development practices.
Why Are You Using Vintage Software Development?
In the 1970s, project timelines at IBM were in years, versus the months or even weeks most modern software teams work in. Practices like automated testing didn’t exist, and software engineering was careful and methodical.
There’s a lot to learn from historical practices. But unfortunately, modern teams often take the worst practices. One of the things we should have left in the past is mandatory code review.
Here’s the problem. When a team requires that each pull request have a detailed code review, it often causes more problems than it solves. You know how they say, “If you don’t have something nice to say, don’t say anything at all”? Well, that’s not an option when “review” is mandatory. So you look for the low-hanging fruit. Variable names, whitespace, style.
This nitpicking is going to cause friction within the team. It also often misses the big picture. The focus of a mandatory code inspection is more about style than how the new code plays with the rest of the code.
And it takes time away from more important things. I can’t tell you how often I’ve been blocked because my pull request was stuck in “code review.” It reduces the agility of teams and negates the benefits of continuous integration.
Code Inspection Scary Stories
One reason the “code inspections” term raised my hackles is because of an experience I had on a team that required all pull requests to have a “code inspection.” When you put your pull request in, a lead developer would assign you a reviewer. But the reviewers were doing development as well as review, so they were really busy. And some were faster at it than others. They got to the code out of order. They all had different standards. There were many days where, by the time it was my code’s turn for review, it needed more work?someone had added something new that they never would have added if my code had been there…and that something broke my code. The integration here was the opposite of continuous. It came in fits and starts.
Because of this, people would bother the reviewers on instant messenger. They’d get barraged with, “Hey, when are you reviewing my code???” Or they’d get mad when one person’s code got reviewed before another’s. It was not a pleasant environment.
Since the pull requests in that team were small?because that’s one of the practices of continuous integration?almost all of the “review” was about things like variable names. There was so much tension, and it was for THAT?
And it all could have been avoided.
Don’t Waste Time Doing What an Automated Tool Can!
Give that nitpicky stuff to an automated static analysis code review tool like Code.It.Right., which can provide instant feedback. This gives a level of speed and consistency you can’t have with human?feedback. And humans can focus on what they do best instead of on the minor issues an automated tool can catch.
What’s awesome about Code.It.Right is it’s built right into your code-writing environment. So no glancing over at terminals or waiting for tests to run. Over time, as a developer using these tools, I also found the continuous instant feedback helped teach me to make fewer small mistakes in the first place.
There’s Still a Key Place for People in the Process!
“Automated” doesn’t mean there’s no people component at all, though. It’s key to work with your team to bring automated code review into your process. One reason these tools aren’t more widely used is that people have bad experiences with them. On one open-source project, we started using an automated code review without really thinking it through. The list of irrelevant errors it came up with was a major source of annoyance. Over time, people just ignored them.
Automated tools should be deployed with care or they can become more of a nuisance than a help. It’s a real hassle if you suddenly find out a bunch of code you worked hard on is error-city. Or that suddenly conventions you relied on, like single-line if statements,?are marked as errors when you’ve been merrily using them for ages. Not a fun time.
That’s why it’s key that you work to determine the coding standards that automated code review operates on. A misconfigured automated review that provides too much “noise” quickly just gets ignored. A developer should never learn you have a new standard for naming variables from an automated tool. Also, you should make sure not to configure checks that are irrelevant to what you’re doing or have your tool check code that it shouldn’t, such as outside dependencies.
Working on this as a team ensures buy-in from everyone. It also makes sure you’re not following rules that don’t fit your needs. You might dread the debates this brings up, but it’s better to get them over with and settled?than have them each time code gets pushed.
Automated Code Review Is the Future
Adopting automated code?review doesn’t mean getting rid of the pull request review entirely. Keep it if you need it. But make sure it’s clear what reviewers are and aren’t looking for so it remains a check and not a burdensome obstacle. As for larger code reviews, it’s okay to keep them, but make sure they’re focused on the big picture of the software as a whole. And the review should be a team effort that’s collaborative.
In the future, I’m going to bet that “code inspections” remain in the 70s and automated code review will be as common as test-driven development. It’s just good business sense to automate when you can.