A while back, I wrote on the subject of code review horror stories.? This was mostly for fun, but it does speak to an important truth.? Code reviews can be pretty contentious, unpleasant, and even counterproductive when done wrong.
I could spend a bunch of time today talking about how to do them right.? But I won’t.? Instead, I’ll talk about ideas for how to automate your way around possible points of contention.? I’m nothing if not a software developer at heart.
Automated Code Review Goes Beyond the Default Rules
You might be wondering how one “automates around” contentious code reviews.? Perhaps you think I’m proposing some kind of pie-in-the-sky scheme to say, “Alexa, review my code.”? But no.? I’m talking about extending CodeIt.Right’s automated review capabilities.
I have a long-running series on this blog, explaining CodeIt.Right’s automated code review rules.? In those posts, I talk about how you should be very deliberate about either heeding the tool’s advice, or deciding to ignore/deactivate a given rule.
But I never really talk about the idea of adding your own rules.
And that’s what we’re going to do today.? You don’t have to use a tool like CodeIt.Right only for its out of the box capabilities when it offers you a rich customization platform?backed by a fully featured API.? You can write your own rules.
And you?should?write your own rules.? Developers waste an awful lot of time in code reviews endlessly rehashing checks that could be automated easily.? And just because it’s something perhaps more specific to your team that?hasn’t?yet been automated doesn’t mean that you can’t automate it?and pretty easily, at that.
So let’s take a look at five ideas for rules you should add to your CodeIt.Right profile.
1. Check for Naming That Your Team Wants to Avoid
Let’s start things off with a bang.? If I were writing a “CodeIt.Right Rules” to impose my will on other people, here are three that I’d personally write:
- Throw an error for class names containing “Helper.”
- Throw an error for class names containing “Manager.”
- And finally, throw an error for class names containing “Utils.”
Why?? Because classes like these inevitably become junk drawer nightmares that spit in the face of SOLID’s single responsibility principle.? I can’t recall ever looking at a class with this name and thinking, “That’s a well-considered object graph.”
Disagree with me intensely?? That’s perfectly fine because I’m not writing the rules for your team.
But odds are that you have some concern like this.? Perhaps there’s some naming scheme that your team wants to move away from or discourage.? At code review, you hear something like, “How many times do we have to say that we’re not calling those adapters anymore? They’re facades.”
The answer to the question is “zero more times.”? Stop telling people this and add a CodeIt.Right profile rule to check for the name “adapter” in classes and throw a warning or error.
2. Ensure That You’re Putting Classes in the Right Folders/Namespaces
“The view models go into the Presentation assembly’s ViewModels folder!”
If I think back a bunch of years to working on a team maintaining a WPF application, I remember hearing something like this during code reviews.? Oh, sure, you could find this information in a big thick coding standards document that no human ever read cover to cover.? But that didn’t stop it from coming up constantly in code reviews.
At first blush, this might seem like another variant on my earlier naming concept.? But it’s subtly different.? This has to do with organization more so than the naming.
To wit, you might call your view models CustomerScreenViewModel, or you might call them CustomerScreenVM or something.? It’s not really about the naming as much as it is about where you put them.? And so the implementation of the rule is going to vary accordingly.? You might identify view models by a common base class or a common method invocation.? Or you might identify them by name.
But what you’re doing with the rule is detecting where they’re going, and if that’s the wrong place, you’re raising a warning.? Let people figure this out in the output window rather than in a conference room or by poring over some dusty Word document.
3. Enforce Architectural Dependency Rules
Are you ever looking through a codebase and suddenly find yourself incensed by a dependency?? Why does the code behind directly use a database driver?? Why are we calling a web service in the domain assembly?? It’s the kind of thing you catch at code reviews.? Maybe.? Or maybe you don’t.
In either case, it’s the kind of thing that you can use CodeIt.Right to enforce.? You can detect the usage of certain assemblies at the assembly, namespace, type, or method level as appropriate.? And then you can similarly set rules as appropriate for a variety of usage targets.
This allows you to detect and warn about the scenarios that I listed and about plenty of others besides.? This is a far superior approach than doing it at code review because developers get nearly instantaneous feedback about problematic dependency introduction, causing them to internalize the architecture.
4. Avoid Anti-Patterns in the Codebase
As I did when discussing issue #1, let me illustrate this with something that might annoy you, depending on your feelings on the matter.? I really don’t like the singleton design pattern.? Every time I see it used, it offers its static instance method so that you can access a tortured mass of unmaintainable global state below.
Your mileage may vary.? But what won’t vary is that I could use CodeIt.Right to detect singletons.? I could look for public static properties that returned an instance of the containing class.? Or maybe public static methods or properties called “Instance.”? I could choose the implementation that made the most sense.
If you disagree with me (or agree with me and want to expand usage scenarios), you could detect whatever you consider to be anti-patterns.? Maybe you don’t like service locators or the observer pattern.? You could write your own detection logic to look for them and toss information or warnings when they occur.
5.? Make Sure Your Unit Tests Are Actually?Testing Something
I’ll close with something near and dear to my heart.? All too often I see upper management decide to create test coverage mandates.? This creates a predictably-pointy-haired-boss moment wherein the development team does, in fact, optimize…for test coverage.
When you look in a codebase like this, you see a predictable pattern of relatively meaningless “coverage tests,” designed not to assert anything but to raise the coverage statistic.? I find this totally understandable in response to poorly conceived incentives.? But it does create a mess in the codebase.? Of course, you might also have a mess of poorly written tests due to inexperience, not to game some metric.
In either case, you can’t have this.? If you’re going to have and run unit tests, they need to have assertions.? So you can write a CodeIt.Right rule that identifies test methods and makes sure that they’re asserting something.? And before you point out relative edge cases like querying a mock object or expecting an exception, you can include that under the general heading of “asserting” and test for that as well.? Think of having a rule entitled “unit tests should test something.”
Use This to Get the Creative Juices Flowing
I deliberately included a couple of strong opinions of my own here to force the issue a bit.? The point here is, yes, to suggest five things that might make for good CodeIt.Right profile rules to add.
But the broader point is to get you thinking about specific situations that make sense for your codebase.? So go back to your own codebase and look for repetitive sorts of mistakes that your team makes and brings up constantly at code review.? And then start to think about how you might automate a check for them.? Doing that can not only make you more efficient, but it can also save you from having code review horror stories told about your team.
Learn more how CodeIt.Right can automate your team standards and improve code quality.
1 Comment. Leave new
[…] I wrote a post for SubMain about custom automated code review rules you can add to CodeIt.Right. […]