Refactor Money with TDD - 01

Refactor Money with TDD - 01

BONUS TECHNIQUES

Let's look at some bonus techniques I use when working in projects.

  • PrivateCtor - Utility class to access private constructors
  • No Primitives - Exchange Rate returns a double
  • MicroObjects - We haven't hit the size limit yet; let's explore that.
  • ToSystemType - Not mentioned during this, but a utility I use to simplify the AsString behavior
  • ADD THE ONES I'M FORGETTING AT 2 AM
  • ...

We'll start with the PrivateCtor.

Public but not to use

In our Money object, we currently have two constructors, one public and private.

protected Money(double amount, Currency currency) : this(amount, currency, new ExchangeRates())
{ }

private Money(double amount, Currency currency, IExchangeRates exchangeRates)
{
    _amount = amount;
    _currency = currency;
    _exchangeRates = exchangeRates;
}

The private one ensures we're only interacting with the interface IExchangeRates while the public one uses constructor chaining to pass in a default implementation. I loathe dependency injection frameworks; this is my technique. I'll talk about this later.

I built systems like this, the first one I built, I had to make the private constructor public to be able to effectively test it. Right now, as I've called out, this constructor isn't accessible via testing, so we're tightly coupled to our hard coded exchange rates. What if that went to the internet to get the current rates? All of our tests would suddenly be coupled to network calls... that's not good. In fact, that's really bad.

I'd much rather have this constructor be public than have external dependencies in the tests. And that's what I did. I didn't like it, but I did it. It was more important to have quick and effective tests than private constructors.

When we'd create objects, there'd always be multiple constructor options. As the system grew, it became a little harder to be sure we're getting the right one. Normally we'd just assume the smallest, but... Having the extra options there that we should never be using... felt very wrong.

Through a bunch of research and experimentation I came up with what I named the PrivateCtor for the obvious reason that it enables us to access the private constructors.

Here it is in all it's glory

/// <summary>
/// Creates instance of a class from a private constructor.
/// <example>
/// Usage:
/// <code>
/// ClassToInstantiate newInstance = new PrivateCtor&lt;ClassToInstantiate>, "ValueForFirstCtorArg", valueForSecondCtorArg)
/// </code>
/// </example>
/// </summary>
public sealed class PrivateCtor<T>
{
    /// <summary>
    /// Convertor to specified type T.
    /// </summary>
    /// <param name="origin"></param>
    public static implicit operator T(PrivateCtor<T> origin) => origin.Object();

    private readonly PrivateConstructorInfo _privateConstructorInfo;
    private readonly object[] _args;

    /// <summary>
    /// Initializes a new instance of the <see cref="PrivateCtor{T}"/> class.
    /// </summary>
    /// <param name="args">The arguments that will be provided to the default constructor.</param>
    public PrivateCtor(params object[] args) : this(args, new PrivateConstructorInfo(typeof(T), args)) { }

    private PrivateCtor(object[] args, PrivateConstructorInfo privateConstructorInfo)
    {
        _args = args;
        _privateConstructorInfo = privateConstructorInfo;
    }

    private T Object() => (T)((ConstructorInfo)_privateConstructorInfo).Invoke(_args);
}

This is on my github as part of my IMock testing framework.

OK - Let's address why I have my own testing framework... Yeah. I built a UWP app. It didn't allow regular testing frameworks that used reflection to build the mock objects. I needed to be able to mock objects to test effectively... and could not.

So... I built my own. I had to.

I still really like using this one, and I'll slip it into the list of things I show as I evolve the code further using my additional techniques and tools.

I don't expect a lot of people to use the IMock framework because other frameworks are more prevalent, but ... I have an affinity for it.

Let's continue on with the PrivateCtor though.

Let's add it to our code and create a test for Money using it. ... Because I don't want to piecemeal in the various pieces just for the PrivateCtor, I'm going to add the IMock Nuget package (Fyzxs.InterfaceMocks) to my code.

Whoops, not Money

Hmmmm.... I've been talking about doing this to money, but since it's become abstract, that doesn't work.

Fortunately our ExchangeRates can serve the purpose. We'll pivot to that after we look at why we don't can't really use Money for that.

The biggest reason is that it's abstract we can't instantiate it. We'd be fine making the private constructor protected and allowing derived classes to implement it and expose it via their own public constructor. So, if I want to test a Money derived class with a different exchange rates, i'd provide a constructor tweak it like that.

With ExchangeRates it's not the same. it's a sealed... OK, now it's a sealed class. So we can't ever expose the ability to change the exchange rates without making that constructor public, which we do not want.

public ExchangeRates():this(StaticMap()){}

private ExchangeRates(Dictionary<Currency, Dictionary<Currency, double>> map) => _map = map;

I'll write up the test and we can discuss it.

[TestMethod]
public void UseDifferentExchangeRates()
{
    //Arrange
    var mockRates = new Dictionary<Currency, Dictionary<Currency, double>>
    {
        { DefaultCurrency.UsDollar, new Dictionary<Currency, double>
        {
            { DefaultCurrency.KoreanWon, 10 }
        } }
    };

    ExchangeRates subject = new PrivateCtor<ExchangeRates>(mockRates);

    //Act
    double actual = subject.From(DefaultCurrency.UsDollar).To(DefaultCurrency.KoreanWon);

    //Assert
    actual.Should().Be(10);
}

This is pretty small. The biggest bloat is constructing the exchange object. All we need to do is pass in the objects to the PrivateCtor constructor and it'll find a matching constructor in the generic type provided and instantiate a new object based on that. It uses implicit casting operators to give us back the generic type.

ExchangeRates subject = new PrivateCtor<ExchangeRates>(mockRates);

It's a small little utility that allows us to keep private constructors private, but still able to test them. Of the many guides I use, "If it can't be tested, it can't be trusted" is one of them. My code does not get to be "untestable". Untestable code is a bad design.
Is it bad to do this? ... Ish. It's reflection, and we should never use reflection... in code. Tests are a little special.

I won't use reflection to access anything other than a private constructor for tests. If there's a method that's so important you have to write tests against it; it should be it's own class.

If I had a bunch of exchange rate types, I'd have ExchangeRates be abstract and create specific implementation for each exchange rate type. Ya know what... let's do that.

public sealed class InMemoryExchangeRates : ExchangeRates
{
    public InMemoryExchangeRates() : base(StaticMap()) {}

    private static Dictionary<Currency, Dictionary<Currency, double>> StaticMap()
    {
        return new Dictionary<Currency, Dictionary<Currency, double>>()
        {
            { DefaultCurrency.UsDollar, new()
            {
                { DefaultCurrency.UsDollar, 1.0 },
                { DefaultCurrency.KoreanWon, 1100 }
            } },
            { DefaultCurrency.KoreanWon, new() { { DefaultCurrency.KoreanWon, 1.0 } } },
            {
                DefaultCurrency.Euro, new()
                {
                    { DefaultCurrency.Euro, 1.0 },
                    { DefaultCurrency.UsDollar, 1.2 }
                }
            }
        };
    }
}

This is our InMemoryExchangeRate. We can give is an informative name.

Since we've also made our ExchangeRates, some tests broke, we'll get those switched over to the new class, at least for now.

Overall, that makes the PrivateCtor no longer needed to test this. We may not need it in this project. That's fine. I have no problem accepting, and will advocate, that needing to use something like the PrivateCtor is in fact an indicator of a design issue. The tests are telling us that something is wrong. Sometimes it's small enough to not worry about it.

For example - What's worse, a single test with the PrivateCtor usage, or having two classes to represent a single idea?

public abstract class ExchangeRates : IExchangeRates
{
    private class InternalTo:IExchangeRateTo
    {
        private readonly Dictionary<Currency, double> _map;

        public InternalTo(Dictionary<Currency, double> map)
        {
            _map = map;
        }
        public double To(Currency currency)
        {
            if (!_map.ContainsKey(currency)) throw new NoExchangeRateAvailableException();
            return _map[currency];
        }
    }

    private static Dictionary<Currency, Dictionary<Currency, double>> _map;

    protected ExchangeRates(Dictionary<Currency, Dictionary<Currency, double>> map)
    {
        _map = map;
    }

    public IExchangeRateTo From(Currency currency)
    {
        if (!_map.ContainsKey(currency)) throw new NoExchangeRateAvailableException();
        return new InternalTo(_map[currency]);
    }
}

public sealed class InMemoryExchangeRates : ExchangeRates
{
    public InMemoryExchangeRates() : base(StaticMap()) {}

    private static Dictionary<Currency, Dictionary<Currency, double>> StaticMap()
    {
        return new Dictionary<Currency, Dictionary<Currency, double>>()
        {
            { DefaultCurrency.UsDollar, new()
            {
                { DefaultCurrency.UsDollar, 1.0 },
                { DefaultCurrency.KoreanWon, 1100 }
            } },
            { DefaultCurrency.KoreanWon, new() { { DefaultCurrency.KoreanWon, 1.0 } } },
            {
                DefaultCurrency.Euro, new()
                {
                    { DefaultCurrency.Euro, 1.0 },
                    { DefaultCurrency.UsDollar, 1.2 }
                }
            }
        };
    }
}

This could very easily be early abstraction. We only have A SINGLE example... hard to extract generic stuff out of one example. I mean... yes... in this case we're extracting what's being provided through the constructor... so probably stable.

For the system though`? Is is worth it right now?

It depends.

So, we could use either technique depending on what fits the system better. I tend to not split a class into the abstract and derived if there's just one example. Though, the "ExchangeRates" in the previous form would be MUCH better named as InMemoryExchangeRates. It'd be more accurate. Then the PrivateCtor would come back in to support out decoupling from concrete values.

I'm not sure what I like more right now. I'll leave it as is. We have some other refactoring we can do to see how the system transforms. Those might tell us which would be a better way to go.

Summary

We looked at an example of using PrivateCtor and how it highlighted a design smell.

Tests are fantastic for showing us those code smells. Sometimes we look on how to fix them, othertimes we accept the smell and keep it identified in the tests. Areas PrivateCtor is useful are often code smells that can be refactored into better designs. I don't think always but it is always a smell. Sometimes the smell is OK and we can wait until the system shows us a better way. That might be never, it might be a minute from now.  Either is OK.

Next Up

Looking at the list of techniques I want to share; Next up is... removing more primitives. While not really a "technique" it's an important thing to continually review the code for.

Show Comments