Redeveloping WordTutor
Static Analysis

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.

FxCop

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:

[assembly: SuppressMessage(
    "Design",
    "CA1034:Nested types should not be visible",
    Justification = "This project uses nested types to structure unit tests and share fixture setup.")]

By convention, these end up in GlobalSuppressions.cs.

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:

[assembly: SuppressMessage(
    "Naming",
    "CA1707:Identifiers should not contain underscores",
    Justification = "This project uses underscores to separate clauses of test names")]

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:

[Fact]
public void GivenUppercaseSpelling_ReturnsTrue()
{
    var spelling = _word.Spelling.ToUpper(CultureInfo.InvariantCulture);
    _word.HasSpelling(spelling).Should().BeTrue();
}

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:

[SuppressMessage(
    "Globalization",
    "CA1308:Normalize strings to uppercase",
    Justification = "No round trip happening here, just a test")]
public void GivenLowercaseSpelling_ReturnsTrue()

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.

Roslynator

The open source Roslynator project is a fantastic collection of analysers, refactorings and fixes. We’re going to add both Roslynator.Analyzers and 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’.

The interface IEquatable<T> declares the parameter name for Equals() as 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.

[SuppressMessage(
    "Maintainability", 
    "RCS1168:Parameter name differs from base name.", 
    Justification = "The name 'word' better identifies the passed value")]
public bool Equals(VocabularyWord word)

RCS1123: Add parentheses according to operator precedence.

I’m a little conflicted by this message. It’s complaining about this code:

public override int GetHashCode()
{
    unchecked
    {
        int hash = 17;
        hash = hash * 23 + Spelling.GetHashCode();
        hash = hash * 23 + Phrase.GetHashCode();
        hash = hash * 23 + Pronunciation.GetHashCode();
        return hash;
    }
}

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.

In the 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.

[SuppressMessage(
    "Redundancy",
    "RCS1212:Remove redundant assignment.",
    Justification = "Treating each step the same aids in readability.")]
public override int GetHashCode()

RCS1118: Mark local variable as const.

In this code, I modified newName to be a constant:

[Fact]
public void GivenName_ReturnsSetWithNewName()
{
    const string newName = "Not the same";
    var set = _set.WithName(newName);
    set.Name.Should().Be(newName);
}

And we’re done! Two useful static analysis tools are now working with us, helping us to spot issues before they turn into bugs.