DEV Community

Cover image for Improve Code Quality with BannedSymbolAnalyzers
Max Hamulyák
Max Hamulyák

Posted on • Originally published at kaylumah.nl on

Improve Code Quality with BannedSymbolAnalyzers

There are many aspects of code quality; testability and consistency come to mind. I don t find it hard to imagine that the longer a project takes with multiple engineers that work on it, the more inconsistencies are in your codebase.

Thanks to the combination of .editorConfig and Roslyn Analyzers, managing this for your team is easier. Recently, however, I was required to create a custom check for my code. A quick google search pointed me to an article from Meziantou which mentioned the "Banned Symbol" Roslyn Analyzer.

Scenario

Before diving into this Analyzer, let's take a few steps back and do a quick scenario sketch. Imagine you work for a company that allows funds transfer between two parties. Since your company needs to make money, you charge a small fee.

public class FeeCalculator : IFeeCalculator
{
    private const decimal FeePercentage = 0.12M;
    private const decimal MinimumCharge = 0.50M;
    private const decimal PriorityFeePercentage = 0.25M;
    private const decimal PriorityMinimumCharge = 7.50M;

    public decimal Calculate(decimal baseAmount, bool isPriority = false)
    {
        if (isPriority)
        {
            return InternalCalculate(baseAmount, PriorityFeePercentage, PriorityMinimumCharge);
        }
        return InternalCalculate(baseAmount, FeePercentage, MinimumCharge);
    }

    private static decimal InternalCalculate(decimal amount, decimal percentage, decimal minimumFee)
    {
        var calculatedFee = amount * (percentage / 100);
        if (calculatedFee < minimumFee)
        {
            return minimumFee;
        }
        return calculatedFee;
    }
}

Enter fullscreen mode Exit fullscreen mode

The company decided to offer "Monday Madness" at a heavily discounted fee as a special offer. The implementation would look similar to the snippet below.

public class DatedFeeCalculator : IFeeCalculator
{
    private const decimal DiscountedFeePercentage = 0.07M;
    private const decimal FeePercentage = 0.12M;
    private const decimal MinimumCharge = 0.50M;
    private const decimal PriorityFeePercentage = 0.25M;
    private const decimal PriorityMinimumCharge = 7.50M;

    public decimal Calculate(decimal baseAmount, bool isPriority = false)
    {
        if (isPriority)
        {
            return InternalCalculate(baseAmount, PriorityFeePercentage, PriorityMinimumCharge);
        }

        if (DateTime.Now.DayOfWeek == DayOfWeek.Monday)
        {
            return InternalCalculate(baseAmount, DiscountedFeePercentage, MinimumCharge);
        }
        return InternalCalculate(baseAmount, FeePercentage, MinimumCharge);
    }

    private static decimal InternalCalculate(decimal amount, decimal percentage, decimal minimumFee)
    {
        var calculatedFee = amount * (percentage / 100);
        if (calculatedFee < minimumFee)
        {
            return minimumFee;
        }
        return calculatedFee;
    }
}

Enter fullscreen mode Exit fullscreen mode

Take notice of line 16 which now uses DateTime.Now; the problem that now arises is: how do we test this code?

Why testing DateTime is hard

The following test only results in a green build on a Monday, which is excellent if we release every week just before "Monday Madness" but not so great on every other day.

[Fact]
public void Test2_Discounted()
{
    IFeeCalculator calculator = new DatedFeeCalculator();
    var fee = calculator.Calculate(10_000M, false);
    fee.Should().Be(7.00M); // note only on Mondays it's 7.00; every other day its 12.00
}

Enter fullscreen mode Exit fullscreen mode

We could mitigate by skipping the offending test every day that is not Monday like this; problem solved, right?

[SkippableFact]
public void Test2_Discounted_Alternative()
{
    Skip.If(DateTimeOffset.Now.DayOfWeek != DayOfWeek.Monday);
    IFeeCalculator calculator = new DatedFeeCalculator();
    var fee = calculator.Calculate(10_000M, false);
    fee.Should().Be(7.00M);
}

Enter fullscreen mode Exit fullscreen mode

Well, no, not actually. What we want is to decouple our code from statics like DateTime.Now by putting them behind an interface. By providing an interface implementation, we can stub a static reference. In an ideal world, this interface would already exist, similar to ILogger in Microsoft.Extensions. For some background reading on why it does not yet exist, see this GitHub Issue.

Updated Scenario

In its most simple from the SystemClock can look like the snippet below.

public interface ISystemClock
{
    DateTimeOffset Now { get; }
}

public class SystemClock : ISystemClock
{
    public DateTimeOffset Now => DateTimeOffset.Now;
}

Enter fullscreen mode Exit fullscreen mode

Our updated scenario looks like this:

public class SystemClockFeeCalculator : IFeeCalculator
{
    private const decimal DiscountedFeePercentage = 0.07M;
    private const decimal FeePercentage = 0.12M;
    private const decimal MinimumCharge = 0.50M;
    private const decimal PriorityFeePercentage = 0.25M;
    private const decimal PriorityMinimumCharge = 7.50M;

    private readonly ISystemClock systemClock;

    public SystemClockFeeCalculator(ISystemClock systemClock)
    {
        this.systemClock = systemClock;
    }

    public decimal Calculate(decimal baseAmount, bool isPriority = false)
    {
        if (isPriority)
        {
            return InternalCalculate(baseAmount, PriorityFeePercentage, PriorityMinimumCharge);
        }

        if (systemClock.Now.DayOfWeek == DayOfWeek.Monday)
        {
            return InternalCalculate(baseAmount, DiscountedFeePercentage, MinimumCharge);
        }
        return InternalCalculate(baseAmount, FeePercentage, MinimumCharge);
    }

    private static decimal InternalCalculate(decimal amount, decimal percentage, decimal minimumFee)
    {
        var calculatedFee = amount * (percentage / 100);
        if (calculatedFee < minimumFee)
        {
            return minimumFee;
        }
        return calculatedFee;
    }
}

Enter fullscreen mode Exit fullscreen mode

With the use of a TestSystemClock or a Moq, we can test our behaviour every day of the week. See, we are improving quality already. In a previous article, "Adventures with Mock" you can read more about my preferred way of creating mocks.

public sealed class SystemClockMock : Mock<ISystemClock>
{
    public SystemClockMock SetupSystemTime(DateTimeOffset systemTime)
    {
        Setup(x => x.Now).Returns(systemTime);
        return this;
    }
}

Enter fullscreen mode Exit fullscreen mode

Thanks to SystemClockMock I can now change the current date for the test.

[Fact]
public void Test3_FakeClock_Monday()
{
    var clock = new SystemClockMock()
        .SetupSystemTime(new DateTimeOffset(new DateTime(2022, 1, 31)));
    IFeeCalculator calculator = new SystemClockFeeCalculator(clock.Object);
    var fee = calculator.Calculate(10_000M, false);
    fee.Should().Be(7.00M);
}

[Fact]
public void Test3_FakeClock_Tuesday()
{
    var clock = new SystemClockMock()
        .SetupSystemTime(new DateTimeOffset(new DateTime(2022, 2, 1)));
    IFeeCalculator calculator = new SystemClockFeeCalculator(clock.Object);
    var fee = calculator.Calculate(10_000M, false);
    fee.Should().Be(12.00M);
}

Enter fullscreen mode Exit fullscreen mode

Force Wrapper over Static Reference

Now that we have our SystemClock, how do we make sure every dev in our team uses it over just calling DateTimeOffset.Now?

Finally, our Roslyn Analyzer comes into play. We can use Microsoft.CodeAnalysis.BannedApiAnalyzers, which triggers the build warning RS0030. I prefer to enable these warnings on every project under src, so I use a Directory.Build.props file to install the analyzer via NuGet.

<Project>
  <Import Project="../Directory.Build.props" />
  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.3.2">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
    </PackageReference>
  </ItemGroup>
  <ItemGroup>
    <AdditionalFiles Include="$(MSBuildThisFileDirectory)/BannedSymbols.txt" />
  </ItemGroup>
</Project>

Enter fullscreen mode Exit fullscreen mode

All that remains is to create a file called BannedSymbols.txt with the following content

note : I also blocked the use of DateTime in favour of DateTimeOffset.

T:System.DateTime;Always use System.DateTimeOffset over System.DateTime
P:System.DateTimeOffset.Now;Use ISystemClock.Now instead

Enter fullscreen mode Exit fullscreen mode

From this point on every use of DateTimeOffset.Now results in the following error: error RS0030: The symbol 'DateTimeOffset.Now' is banned in this project: Use ISystemClock.Now instead. Which in my opinion is pretty cool :)

Closing Thoughts

Even if the system used in today's example is fictional, I think the BannedSymbolAnalyzers is a compelling package to include in your toolbelt. At the very least, I will use it to force DateTimeOffset over DateTime. Situation allowing I will also push my wrappers over static references to improve testability.

As always, if you have any questions, feel free to reach out. Do you have suggestions or alternatives? I would love to hear about them.

The corresponding source code for this article is on GitHub.

See you next time, stay healthy and happy coding to all 🧸!

Additional Resources

Discussion (2)

Collapse
kaylumah profile image
Max Hamulyák Author

Are you interested in sharing your thoughts on my work? Consider commenting on this github.com/kaylumah/kaylumah.githu... GitHub issue. That way, your reactions will also be available on my blog!

Collapse
davidkroell profile image
David Kröll

That's a nice way to get rid of non-deterministic code! With this method you shouldn't miss any usage.