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.
Learn more how CodeIt.Right can help you automate code reviews and improve the quality of your code.
9 Comments. Leave new
Humans haven’t changed much. People make the same mistakes now as they did back then (choose your decade!). They even make the same mistake that thinking that they are better now than those they replace. They also continue to mistakenly view the activity as blame, rather than learning, education and community. People don’t agree on the conventions and standards either, so automated tools become a further source of contention. Aren’t people great… Working together works best (until you meet the other team tunnelling in the opposite direction)!
“Code inspections” are not the same as “peer review”. Code inspections are highly formal affairs with a “reader”, “author”, “reviewers”, and a “scribe”. Anyone who has participated in them realize what a major PITA they are. Academic research shows they are only marginally more effective that a peer review with a single reviewer. Automated tools (like Code.It.Right) should always be used in advance of any human spending time reviewing code.
Many companies do fail to use the learning that is embedded in the set up of the ‘inspection’ process, from the number (sufficient) of participants and their (directed) roles, which brings out productivity if embraced.
Unfortunately, as you say, far too often these things become rote ‘going through the motions’ activities that no-one understands nor cares about which then become the usual politics.
Also the method predates the capabilities that tools like Git (which distributes control to the user) can provide. I’m just about old enough to understand where the change control processes come from (as if you can control change..), and hence why it will tend to fail in a modern software environment (zero cost of replication & machining of ‘material’)
The real reason for code inspections or code reviews is to bring the best ideas to light, and sometimes to find better, alternative algorithms, or methods to achieve the same result. And as more people participate they acquire those skills and practices for themselves, no matter how disdaining they may be of the process. It is difficult to face criticism, I know, but it is a growth mechanism, and does everyone present good, if done properly. Don’t take comments personally, don’t make personal comments, and do try to see how each suggestion improves the process, or how each can be handled better. That is the reason that automated tools do not achieve the same results for a company or a team. Ask coaches for football teams how they improve the team!
I agree, a tool does not completely replace the human review – thank goodness for that or we’d all be out of jobs 🙂 The automated tools intent is not to improve the team relations per se but to lower the human effort by finding many of the issues without spending the human time. And that is the job they do. Also, they do help addressing the “Don?t take comments personally, don?t make personal comments” aspect you mention – a tool provided not biased but (rather) objective feedback.
Unfortunately, as a coder myself, I know that code itself carries inherent bias, not by design, but rather as a byproduct of the means and methods that the coder herself embodies. We are all biased, it is in our nature. learning is a means of using bias to produce effective work. A good example of learning as biased is in the AI program being developed by a black lady that did facial recognition, when she learned it didn’t recognize her face until she put on a white mask. All learning, even by humans is subjective to the material they are taught, and so it is with algorithms we create. Often the bias is not evident in the algorithm, as with machine learning, but rather in the results produced by the one employing the lessons to achieve something.
If you don’t understand the need for good review, either you never met a horrible coder or are a horrible coder yourself… Reviewers are not just supposed to correct typo’s in people’s parameter names, they’re supposed to use their experience to think if they would have implemented something differently, and then use their experience to decide if that means their way is better (in which case they should fail the review) or just different (in which case they should pass it).
I can’t even count the number of times I found bugs, copy-paste mistakes, memory leaks, badly or unintuitively implemented code, inefficient uses of data structures and so on, even when reviewing experienced developer’s codes – and similarly, the code reviews I’ve received gave invaluable tips and tricks for future developments. Some of these things would have been found by unit tests, sure, but not all of them – and definitely none of the ones that make sure your code is readable and maintainable.
I think the main take-away is to do one’s research on a company the has a blunt ‘code inspection’ job requirement, and check that they do it well.
Exactly how they do their reviews is one of those ‘smells’ you will want to check on (among other bandwagon and legacy techniques). Choose to work for a good employer, make sure you ask, and see, how they do it!
“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?”
……
“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.'”
Hmmm…