If we start with the premise that having code reviews?is better than not having them, the next step is to figure out how to make them better. And though we’re all aware that code reviews are a good thing, they can quickly degenerate into toxic scenarios. We don’t want that.
In this post, we’re going to see four code review practices that can make developers feel frustrated and alienated. In the more extreme cases, developers might even leave a company to escape from what they perceive as tyrannical, nonsensical demands. Keep reading to learn what those practices are, how to avoid them, and how to leverage the benefits of automation in your code reviews to prevent many of these toxic scenarios from happening in the first place
Once you know that, you’ll be empowered to turn your code reviews into healthy processes that lead to higher code quality and more happiness for the developers in your team. Let’s get started!
1. Arbitrary Suggestions
I know this might sound somewhat idealistic, but everyone’s opinion in the development team should be held to the same level of scrutiny. What I mean by this is that all code reviewers should be able to justify their suggestions. Seniority in the company doesn’t automatically translate to being right about everything all the time. “Because I said so” is not an argument.
“Because so-and-so famous person said so” isn’t an argument either. Sure, you can and should cite references whenever possible if that helps you make your argument?books, papers, blog posts, what have you. But you shouldn’t rely on those external sources as the argument you’re making. Instead, use them as the starting point from which you build your case.
If all reviewers just do as they please and start asking for changes based only on “this guy on YouTube said so” type of arguments, developers will get alienated pretty quickly. Soon, the situation will devolve into the problem we cover in the next topic: inconsistency across reviewers.
2. Inconsistency Across Reviewers
Let’s say?Anna?name generated by the random name generator?has just joined a new company as a software developer. Anna has experience in several different languages and platforms, but this will be her first C# job. So she’s excited to finally put into practice some of the most interesting C# features she’s read about.
Anna’s First Assignment
One of the C# features Anna likes the most is type inference, so she uses it every time she gets the chance. Here’s an excerpt of the code she wrote in her first assignment:
var end = DateTimeOffset.Now; var start = DateTimeOffset.Now.AddDays(-7); var orders = orderRepository.FindByPeriod(start, end);
She then submitted the code for review. What follows is the feedback related to the code above:
Please change the third line to not use type inference. The usage is OK in the first two lines since the right-hand side of the assignment makes it obvious which types will be inferred. That’s not the case in the third line, so please change it to explicit typing.
Anna then did what the reviewers asked:
var end = DateTimeOffset.Now; var start = DateTimeOffset.Now.AddDays(-7); IEnumerable<Order> orders = orderRepository.FindByPeriod(start, end);
Since everything else seemed OK and all the tests were passing, Anna’s changes were accepted and merged.
Another Review
Not much time after that, Anna submitted more changes for review, this time to a different reviewer. Here’s an excerpt:
var numbers = userInput
.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)
.Select(int.Parse);
And here’s the feedback the reviewer gave her:
Please don’t use type inference in this situation. Sometimes it gets the wrong type and things can get confusing, especially for beginners.
This time, Anna didn’t agree with the feedback; it seemed to her as though the reviewer didn’t really understand type inference. Maybe they were mixing it up with dynamic typing or something like that. However, even without agreeing, Anna went ahead and changed the code to explicitly use IEnumerable<int> instead.
The Last Straw
A few days later, Anna submitted code for review to yet another reviewer. In the code, she used type inference just in the most obvious ways:
var start = userRequest.StartDate;
var end = userRequest.EndDate;
IEnumerable<Order> orders = orderRepository.FindByPeriod(start, end);
var message = $"Your search has returned {orders.Count()} results.";
And here’s the feedback this time:
Don’t ever use type inference! That’s an awful feature that should’ve never made its way into C#! Explicit is better than implicit, always!
That was the last straw. After that, Anna was done with the whole thing: the code review process, and also the company.
Inconsistency in code reviews destroys the team’s morale. Developers start to think that this whole code review thing is a nonsensical waste of time, whose only purpose is to allow some people with more senior status in the company to feel like they have some power. And sadly, that’s often exactly the case.
If you want your team’s code review process to go as smoothly as possible, it must have internal logic and consistency. The team must agree on the definitions of good code and bad code?and write them down. That way, you remove personal opinions from the process, which significantly reduces the likelihood of arbitrary preferences influencing the results of reviews.
And of course, as I’ve mentioned?and will mention again soon?embracing automation can ease this and other pains you might have found in code review. Keep reading to find out how.
3. Excessive Nitpicking About Style Issues
Have you ever spent hours in a coding task, just to have your changes disapproved during code review due to a minor stylistic issue? Maybe you’ve added too many line breaks after a line of code? Put more?or less?space before and after binary operators? And what about the correct placement of commas, parentheses, and brackets? The list could go on and on, but I’m sure you can relate. When that happens, it’s a huge bummer.
Some of you might be thinking that this entry is at odds with the previous topic. There, I suggested that consistency in the code review process?particularly cross-reviewer consistency?is a must. Here, it looks like I’m saying that consistency doesn’t matter that much in regard to cosmetic choices. What’s the deal?
Here’s the thing: your team’s code should look consistent. You shouldn’t be able to distinguish Bob’s code from Alice’s code just by staring at it. And yes, cosmetic/stylistic issues are included in that. So it’s not OK having the following variations in the same codebase:
SomeMethod(param1,param2,param3); SomeMethod(param1, param2, param3); SomeMethod(param1 , param2 , param3); SomeMethod(param1, param2, param3);
You should pick one variation as the standard and follow it, and this should be enforced. But there are better ways of accomplishing this goal than nitpicking during manual code review. Those better ways involve automation. More on this later, so keep reading!
4. Going Against Established and Widespread Conventions
In theory, you and your team could use any coding convention you agree upon. Will the compiler care whether you name a property “BirthDate,” “birthDate,” “birth_date,” or yet another variation? Not a bit. Depending on your Visual Studio/C# version, you’ll get warnings?which you should treat as errors anyway. But nothing stops you from completely ignoring those warnings, so I guess it’s fair to say the compiler doesn’t care a bit about your naming conventions.
So does that mean you can create coding standards using any conventions you fancy? Well, you can, but you shouldn’t. And why is that?
Conventions exist for a reason. Following coding conventions that are used and understood by a considerable portion of developers will lighten the burden of onboarding new developers. Your code won’t look alien to them since they’re already used to that style.
It also becomes easier for external developers to understand your team’s code if you decide, for instance, to open-source some tool you created for internal use and figured would be useful for people outside the organization.
Following standard coding conventions also allows you to leverage the power of automation. For instance, on Visual Studio you can use the CTRL K + D?shortcut to automatically format the open document, using Visual Studio’s convention.
Leverage the Power of Automation to Keep Your Team From Falling Apart
If you can automate something, you probably should do it. I like to say this to remind others and myself of the efficiency gains we get by embracing automation’s benefits. We developers, of all people, should remember that. But you know what they say: “The shoemaker’s son always goes barefoot.”
SubMain’s CodeIt.Right?is a static analysis tool that can help you and your team escape from the traps of manual code review. CodeIt.Right offers you instant feedback about the quality of your code, checking it against a wide variety of rules from many different categories. It can even perform automatic fixes depending on the issue. You could additionally employ another tool to take care of stylistic issues, further freeing your team to focus on higher-level concerns when doing reviews.
Employing automation as described above addresses all of the four points I’ve mentioned in this post. Arbitrariness and inconsistency? Things of the past. With an automated code review tool, you get repeatable, predictable behavior.
Nitpicking about stylistic issues? That’s exactly why you’d want to employ something like StyleCop?because it’s nitpicky. But since it’s a tool and not a person, no feelings will get hurt. And finally, by using CodeIt.Right, you’re guaranteed to follow well-established conventions, since the default coding styles of the tool mimic those of Microsoft.
Embrace automation and get the best of both worlds: the rigor of automatic checking and the flexibility and creativity of the human mind?each?one in the place that suits it better.
Learn more how CodeIt.Right can help you automate code reviews and improve the quality of your code.
