No Primitives - Again

Since we're poking around the ExchangeRates area, one thing has stood out to me.

public interface IExchangeRateTo
{
    double To(Currency currency);
}

This interface has a method that returns a double... a Primitive. That's ... Nooooooo!

What can we do to fix this? Clearly a type of somekind... what kinda... Perhaps an ExchangeRate object?

Let's sketch one out and see what it looks like. It might drive some additional changes to our Exchange Rates data structure since it's kinda built on having a double.

public sealed class ExchangeRate : IExchangeRate{
    private readonly Currency _from;
    private readonly Currency _to;
    private readonly double _rate:
    
    public Money Convert(???) => ???
    
    public override ToString() => $"{_from}->{_to}={_rate}"
}

I like the over all look of this. I don't know what the Convert method will look like yet. I could do a few things, but it's going to have constraints around how Money works right now.

Since we're in Money to do the conversion... can it take the double for amount? That feels really dirty. I feel there may be some callbacks otherwise... Callbacks aren't bad things, I use them alot. The caution I have around them is that they start to get real close to circular dependencies, so it's something we need to keep an eye on. Right now, thinking about how this might play out... No, I haven't written any more code than you see above. This is my real time thinking...

• Money will depend on
• IExchangeRates
• IExchangeRate
  Note: This makes it pretty obvious that IExchangeRates should be renamed. To at least IExchangeRateCollection. Pluralizing something that is a collection makes the difference hard to see. The parallel of Make similar things more similar is Make different things more different. I like to phrase it as Make different things more unlike eachother. Because it breaks the similarity with the first part... Fine. Make different things more different. The names should be more different because the types are different.
• IExchangeRateswill depend on
• IExchangeRate

To do a call back, IExchangeRate would depend on Money and then we'd be crcular. This should be an image of a directed graph... We'll see if I update it to one. Or I stay lazy and not.
In the above sketch of an ExchangeRate class, it is returning a Money, which we can't do. It's that circular dependency.

With this we can see that IExchangeRate cannot take a money object. I don't like data flowing through the system. As Allen Holub has said

it's best to minimize data movement as much as possible. My experience is that maintainability is inversely proportionate to the amount of data that moves between objects. Though you might not see how yet, you can actually eliminate most of this data movement. - https://www.infoworld.com/article/2073723/why-getter-and-setter-methods-are-evil.html

I think about this everytime a method takes or returns a primitive. It's knowingly adding complexity into the system.

Much of my approach to development is - Know it's a problem, but don't solve it prematurely. I don't know how to prevent the Convert method from taking or returning a primitve value. Maybe one will show itself - as we listen to the code - or it won't and we'll accept this smell between two "primitive holders". We have 2 objects that both use primitive values... is it OK for them to exchange primitives? ... Maybe? I haven't thought about it too deeply. I normally only like these ctypes lasses to take primitives in the constructors. Right now, I don't think I'll have a better answer until I get some code in place and see how the system interacts with it.

Missed refactors

... HAHAHA - I missed something that may simplify the problem space.

We have three places in Money that the exchange rate is used. Two of them are identifical. We looked at how to abstract things, but decided it was too early. I really hope you, dear reader, were far sharper than I in the moment and saw that I could just call the other usage.

Now, before we slam me too hard for missing that... I do most of this writing between 11pm and 3am. My sharpness is a little dull being up this long.

Let's look at the Plus and To methods.

public Money Plus(Money addend)
{
    double exchangeRate = _exchangeRates.From(addend._currency).To(this._currency);
    return new InnerMoney(this._amount + (addend._amount * exchangeRate), _currency);
}

public Money To(Currency currency)
{
    double exchangeRate = _exchangeRates.From(_currency).To(currency);
    return new InnerMoney(_amount * exchangeRate, currency);
}

There's a lot of similarities there.... In fact... if we change the Plus method around just a little.

public Money Plus(Money addend)
{
    double exchangeRate = _exchangeRates.From(addend._currency).To(this._currency);
    Money to = new InnerMoney((addend._amount * exchangeRate), _currency);
    return new InnerMoney(this._amount + to._amount, _currency);
}

We can make it OBVIOUS that the first 2 lines are the same as the To method... so yeah... we can just call the To method from our Plus method! ... I feel dumb for not seeing that earlier.

Giving us the resulting Plus method

public Money Plus(Money addend)
{
    return new InnerMoney(this._amount + addend.To(_currency)._amount, _currency);
}

Tests are all still green. A successful refactor... even if a little late.

Seeing this... painfully obviousness... we can use the To method in Equals as well.

private bool Equals(Money other)
{
	return _amount.Equals(other.To(_currency)._amount);
}

Back to Primitive Removal

OK, a nice distraction to simplify things. we're down to a single place we use the double out of the exchange rate. Pretty contained. Is it good enough?

No. At least... not long term.

The use of the exchange rate value is nice and encapsulated. It'd be great if an exchange rate could return a Money. Then the primitives would only be doing stuff IN the exchange rates and not in Money. But then Money can't depend on exchange rates.

.... I spend a lot of time just staring at code I want to change. Stare and think about what I can do. This is leading me back to the same answer I have a lot when I'm dealing with abstract classes the hold primitves... I need protected abstract methods to allow derived classes to define how the primitives are generated. It opens up the primitives to a few more places... but they have to know it somehow anyway. UsDollar currently takes a primitive and passes it into the constructor of Money. It already knows of primitives... this isn't really exposing it any wider. It's giving derived classes more control over how, and when, they produce a primitive value.

Essentially, at the moment our Money class is tightly coupled to the "amount and currency" concept of money. That's our real world... but code doesn't always map directly to the real world, and when we try to force it... the code fights us. The protected abstract generator methods have been a reliable way for me to create a heirarchy of primtiive encapsulating classes that can churn against eachother. This is a very narrow scope of classes. I try not to... but I keep coming back to the pattern; it sovles the problem for me really well.

As we're tightly coupled between what UsDollar wants and how Money implements it... and that Euro and KoreanWon will also need the same treatment... an intermediate abstract class seems reasonable.

public abstract class MoneyOf:Money
{
    protected MoneyOf(double amount, Currency currency) : base(amount, currency){}
} 


public sealed class UsDollar : MoneyOf
{
    public UsDollar(double amount) : base(amount, DefaultCurrency.UsDollar) { }
}
public sealed class Euro : MoneyOf
{
    public Euro(double amount) : base(amount, DefaultCurrency.Euro) { }
}
public sealed class KoreanWon : MoneyOf
{
    public KoreanWon(double amount) : base(amount, DefaultCurrency.KoreanWon) { }
}

We'll break the coupling first via the above changes. A simple pass through initially.

Now we can get to the methods in Money and only have to update MoneyOf for an implementation. I don't think I'll go through every step here; I'll get to the changes and share them.

In Money we end up simplifying the constructors

protected Money() : this(new InMemoryExchangeRates()) { }

private Money(IExchangeRates exchangeRates) => _exchangeRates = exchangeRates;

We replace _amount and _currency with out abstract methods

protected abstract double AmountValue();
protected abstract Currency CurrencyValue();

and follwo with replacing any use of _amount by AmountValue(). Same with _currency and CurrencyValue().

InnerMoney and MoneyOf both need to be updated to implement these abstract methods and return the constructor values.

These methods LOOK like getters... and are pretty close to it with the two examples we have. I hope to make InnerMoney go away and provide examples of why they aren't getters, aside from not being public.

And No, despite some other books, simply removing get from the prefix of a method DOES NOT change the fact it's a getter.

Replace InnerMoney

Money uses InnerMoney in 4 places. Let's replace those. Money is knowing way too much about how the changes of money happen. SURE, they're simple operations, but it still knows to much.

Let's replace the uses of InnerMoney with types that perform that function for us. We'll do Times first since it's pretty simple. So is... DivideMoney... What the hell... That's a horrible name. ESPECIALLY with the principle to Make simiilar things more similar We have Times and Plus and To. DivideMoney should be called Divide. I'll get that commit in real quick. Simple tool rename refactor.

OK, now onto extracting the work of the Times operation.

Which encounters a little bit of a speed bump. Derived classes, much like private things... don't have access to protected things. We can't make it public. That let's data loose into the world. Since we're in C#, and this is one of the reasons I like C#... we'll create some explicit type casting on our base class to facilitate.

public abstract class Money
{
    public static explicit operator double(Money origin) => origin.AmountValue();
    public static explicit operator Currency(Money origin) => origin.CurrencyValue();
    ...
}

Now, we can force a Money instance into a either a double or a Currency. There is an implicit conversion as well, that the compiler will invoke for you; and it's great at times. One of the things I like to enforce is when we are going to do conversions to primitive types, let's make it explicit in the code. Explicit casts are a smell; They're dangerous. When we can have the code present a visual reminder of that, it helps us keep it in mind. For me, it helps me look for ways to remove it, without hiding it.

This allows us to create a TimesMoney class that looks like this

internal class TimesMoney : Money
{
    private readonly Money _origin;
    private readonly int _multiplicand;

    public TimesMoney(Money origin, int multiplicand)
    {
        _origin = origin;
        _multiplicand = multiplicand;
    }
    protected override double AmountValue() => (double)_origin * _multiplicand;

    protected override Currency CurrencyValue() => (Currency)_origin;
}

Which we can replace the contents of Times in Money with returning an instance of this class.

public Money Times(int multiplicand) => new TimesMoney(this, multiplicand);

That's it.

We can do an identical treatment for Divide

public abstract class Money{
    ...
    public Money Divide(int divisor) => new DivideMoney(this, divisor);
    ...
}
internal class DivideMoney : Money
{
    private readonly Money _origin;
    private readonly int _divisor;

    public DivideMoney(Money origin, int divisor)
    {
        _origin = origin;
        _divisor = divisor;
    }
    protected override double AmountValue() => (double)_origin / _divisor;

    protected override Currency CurrencyValue() => (Currency)_origin;
}

And again for Plus... but it uses To... Let's do To first. This one gets a little bigger than TimesMoney and DivideMoney did

public abstract class Money{
    ...
    public Money Divide(int divisor) => new DivideMoney(this, divisor);
    ...
}

internal class ConvertToMoney : Money
{
    private readonly Money _origin;
    private readonly Currency _toCurrency;
    private readonly IExchangeRates _exchangeRates;

    public ConvertToMoney(Money origin, Currency toCurrency) : this(origin, toCurrency, new InMemoryExchangeRates()) { }

    private ConvertToMoney(Money origin, Currency toCurrency, IExchangeRates exchangeRates)
    {
        _origin = origin;
        _toCurrency = toCurrency;
        _exchangeRates = exchangeRates;
    }
    
    protected override double AmountValue() => (double)_origin * _exchangeRates.From((Currency)_origin).To(_toCurrency);

    protected override Currency CurrencyValue() => _toCurrency;
}

And we stay green.

There's a smidge of complex logic around the AmountValue(). We have to us the explicit casts for both values out of _origin. This is why I like the explicit cast vs the impliciy. We can see exactly what's going on when we're doing these transformations in our code. I prefer implicit if interacting with 3rd party libraries so that area doesn't have the hard cast smell to it. I can't fix 3rd party libraries wanting primitive types.

Once we do Plus - InnerMoney will no longer be needed.

public abstract class Money{
    ...
    public Money Plus(Money addend) => new PlusMoney(this, addend);
    ...
}

internal class PlusMoney : Money
{
    private readonly Money _augend;
    private readonly Money _addend;

    public PlusMoney(Money augend, Money addend)
    {
        _augend = augend;
        _addend = addend;
    }
    protected override double AmountValue() => (double)_augend + (double)_addend.To(CurrencyValue());

    protected override Currency CurrencyValue() => (Currency)_augend;
}

We'll knock these all out into their own files to keep Money.cs nice and clean.

This also allows us to remove the constructor chaining from Money simplifying the class.

Let's look at Money

public abstract class Money
{
    public static explicit operator double(Money origin) => origin.AmountValue();
    public static explicit operator Currency(Money origin) => origin.CurrencyValue();

    protected abstract double AmountValue();
    protected abstract Currency CurrencyValue();

    public Money Times(int multiplicand) => new TimesMoney(this, multiplicand);

    public Money Divide(int divisor) => new DivideMoney(this, divisor);

    public Money Plus(Money addend) => new PlusMoney(this, addend);

    public Money To(Currency otherCurrency) => new ConvertToMoney(this, otherCurrency);

    private bool Equals(Money other)
    {
        return AmountValue().Equals(other.To(CurrencyValue()).AmountValue());
    }

    public override bool Equals(object obj) => ReferenceEquals(this, obj) || obj is Money other && Equals(other);

    public override int GetHashCode() => HashCode.Combine(AmountValue(), CurrencyValue());

    public override string ToString() => $"{AmountValue()} {CurrencyValue()}";
}

To me; this looks pretty minimal and compact and ... except Equals. Equals is doing stuff. Lots of stuff. It needs it's own class. But... how do we create a class that's a bool... yeah. I haven't solved this one yet. It's hacky and some have called it stupid - But look at the Equals method - It knows how to churn Money to determine if they are equal. We've removed the churning to add Money together. Same for Times and Divide: Money no longer has the knowledge of how to do the work - But it sure as hell knows what classes DO know how to do the work. I'm sure there's an earlier example, but from Sandi Metz I get

"I know what I want, you know how to do it". - Sandi Metz

In this case, Money knows what it wants, and it delegates that to the object that knows HOW to do it. Money should not know how to Add, and Multiply, And Divide, and Convert... AND determine Equality. Money should know how to determine equality just as much as it should know how to the rest of those things - Not. At. All.

While the result of this will look ... funny; it keeps our principles in tact, it makes similar things more similar, and it exposes that Booleans are dumb and I wish I had objects like SmallTalk for them.

If you caught that I didn't seal all my classes - Good Job. It's common for me to forget. I will often search the code for public class and internal class to find classes I haven't made sealed or abstract.

One of the other reasons I like to extract this behavior into it's own class - what happens if we update code that forces a change to one of the methods? if we change the exchange rate methods; Money has to change. If we update equality calculation, Money has to change. Money gets as many reasons to change as behaviors it implements. If we want to keep our reasons to change low - Following Single Responsibility; then we have to extract out the behavior so that Money won't change for multiple reasons.

So we create a class to handle equality

public abstract class Money{
    ...
    private bool Equals(Money other) => (bool)new EqualMoney(this, other);
    ...
}
internal sealed class EqualMoney
{
    private const double Tolerance = .0001;
    public static explicit operator bool(EqualMoney origin) => origin.Value();
    private readonly Money _lhs;
    private readonly Money _rhs;

    public EqualMoney(Money lhs, Money rhs)
    {
        _lhs = lhs;
        _rhs = rhs;
    }

    private bool Value()
    {
        double lhsAmount = (double)_lhs;
        Currency lhsCurrency = (Currency)_lhs;
        double convertedAmount = (double)_rhs.To(lhsCurrency);

        return Math.Abs(lhsAmount - convertedAmount) < Tolerance;
    }
}

There's a couple bits that aren't great currently. It has no interface or base class. It's not a Money... it's a ... boolean of some type. That isn't reference to the C# struct Boolean but a boolean value. One of the techniques on the list will provide a base class for this type of class... We're not quite to that, so we'll let it slide.

In money, that we have to create and cast a class... is ugly. We want to return things, not create and "use"; even if it is just a cast like this. Again, I have a solution; which is upcoming.

Files, Files, Everywhere Files

Our file structure is getting pretty noisy now. We should do some organizations.

Some simple file organization is to create folders (which correspond to namespaces) for Exceptions, Monies, Currencies, and ExchangeRates. Since our contrived example has tests and code in the same project; we'll also have a Tests.

What's Next?

... You might guess that it's getting rid of having the IExchangeRateTo returning a double... While you wouldn't be wrong that's still a concern... It's currently isolated to a single object, ConvertToMoney. Until there's more to it... I can't see an good way to remove it. We NEED the value. Aside from an arbitrary object and explicit cast... It feels like overkill for where the code is at.

That's right - Overkill from the dev that just extracted single line methods into classes.

Next

We'll take a quick detour into the ToSystemType abstraction I use  and see how it can apply to our boolean EqualMoney . Aftrer that, I expect we'll go into  a discussion about my Guiding Light Philosophy of "Represent the Concept" which drives a lot of the basis for what I call "MicroObjects".