I’ve long been a fan of static analysis tools. They can act as a co-pilot, keeping an eye on things while you work, helping you to catch common types of problems. Let’s add (and tune!) some tools.
A code analysis tool is a set of opinions about how you should be writing your code. Whenever you import a code analysis tool, you’re importing those opinions. Some developers react negatively when they see the number of violations reported. In most cases, these come down to a difference in opinion, a different in style. The good news is that we’re in charge, and we can tune the tools to dance to our tune.
Back in the days of Visual Studio 2010, fxCop was a commonly used tool for doing static code analysis in .NET Framework projects. The
Microsoft.CodeAnalysis.FxCopAnalyzers NuGet package brings most of those rules forward into new ecosystem of .NET Core.
As expected, as soon as we install the package we start getting a large number of warnings. In my case, I immediately saw over 140 warnings. That’s a bit intimidating. Let’s work through them and see what we can learn.
First we have a warning that’s firing on code in our test project:
CA1034: Do not nest type Empty. Alternatively, change its accessibility so that it is not externally visible.
The rule here is that nested types should never be externally visible. In our case, we’re using nested classes to share test infrastructure. We need them to be public so that the tests can be discovered and run. So we’ll suppress all the warnings from this tool:
By convention, these end up in
When Visual Studio suppresses a code analysis rule, only that particular violation of the rule is suppressed. What I’ve done here is to remove the scope, so it applies project wide. I’ve also added a justification so that future maintainers know why I suppressed the rule. (One of those future maintainers is likely to be me - so I’m leaving clues for my future self.)
CA1707: Remove the underscores from member name WordTutor.Core.Tests.VocabularySetTests.WithName.GivenNull_ThrowsException().
Idiomatic C# code uses camelCase or PascalCase, so this rule suggests that underscores should never be used. We’re using underscores to separate different phrases in our test names, so we need to suppress this rule too:
We’re down to only two remaining messages. That’s over 140 warnings remedied with just two suppressions.
CA1304: The behavior of ‘string.ToUpper()’ could vary based on the current user’s locale settings. Replace this call in ‘VocabularyWordTests.HasSpelling.GivenUppercaseSpelling_ReturnsTrue()’ with a call to ‘string.ToUpper(CultureInfo)’.
This is a real bug. We want our unit tests to run the same regardless of the locale used on a particular machine. The fix is simple:
After fixing a similar warning in
GivenLowercaseSpelling_ReturnsTrue(), I ran a full build … and got a new warning:
CA1308: In method ‘GivenLowercaseSpelling_ReturnsTrue’, replace the call to ‘ToLower’ with ‘ToUpperInvariant’
Reading the documentation page on this rule, we learn that some characters don’t round trip properly when using lowercase. We know that this particular method is safe, because we’re not doing a round trip.
So we’ll suppress just this one warning in the code. There are two ways to do this. The use of
#pragma comments is particularly ugly, so instead we will use an attribute on the method itself:
Using an attribute based suppression turns off that warning for the entire method. If the method is large, this can backfire, suppressing issues that should instead be fixed. I guess this is another reason why methods should be kept small and easy to understand.
The open source Roslynator project is a fantastic collection of analysers, refactorings and fixes. We’re going to add both
Roslynator.CodeFixes to our projects.
After addition, and a full rebuild, we have only a handful of issues to review.
RCS1168: Parameter name ‘word’ differs from base name ‘other’.
IEquatable<T> declares the parameter name for
other. The rule here is that the parameter name of an implementation should match the parameter name from the interface. For this one method here, I disagree, because
word is a better name here than
other. We’ll suppress this one warning.
RCS1123: Add parentheses according to operator precedence.
I’m a little conflicted by this message. It’s complaining about this code:
The precedence of multiplication over addition is a fundamental part of mathematics. I would not expect any experienced developer to have a problem reading and understanding this code.
But not every developer has a formal background, many are self taught. Perhaps my instincts in this area are not reliable.
In the interests of clarity, I’ll change the code by adding the suggested parenthesis.
RCS1212: Remove redundant assignment.
GetHashCode() method shown above, the last assignment to
hash is redundant. We could rewrite the method to avoid the assignment and return the final calculation.
Having each step of the calculation presented in the same way helps someone reading the code understand what is going on. It also makes things easier for someone adding another step to the calculation later on. Let’s suppress this one.
RCS1118: Mark local variable as const.
In this code, I modified
newName to be a constant:
And we’re done! Two useful static analysis tools are now working with us, helping us to spot issues before they turn into bugs.