The Fixing

I don't know what it's gonna take to work through fixing this. It's a pretty big goof to have missed that I need to have a Money object back... Oh well. Let's get to it.

A nice thing is that it's all in the Money class. So... Maybe minimal futzing.

Actually... Ya know what... Let's do the next requirement; BUT - Do it correctly with returning an object, not a value.

It's adding up

5 USD * 2 = 10 USD
10 EUR * 2 = 20 EUR
4002 KRW / 4 = 1000.5 KRW
5 USD + 10 EUR = 17 USD
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD

The next requirement is adding 2 different currencies. I think that's a big of an overstep; so let's add in another requirement.

5 USD * 2 = 10 USD
10 EUR * 2 = 20 EUR
4002 KRW / 4 = 1000.5 KRW
5 USD + 10 USD = 15 USD
5 USD + 10 EUR = 17 USD
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD

This way we can focus just on addition and not exchange rates. Plus... ya know... returning the correct type.

Alrighty, here is our failing test.

[TestMethod]
public void PlusExists()
{
    Money subject = new TestMoney(5, "");
    Money actual = subject.Plus(subject);
}

Which won't compile because it doesn't exist!!! That's right, I keep doing it.

Using the return type and parameter will have the tool generate everything correctly.

A quick easy implementation

public Money Plus(Money addend)
{
    return null;
}

While, normally, I loath null; it gets us to green and we'll be fixing it VERY soon.

Like... Right now soon

[TestMethod]
public void FiveCurrencyPlus10CurrencyShouldBe15Currency()
{
    //Arrange
    Money five = new TestMoney(5, "CUR");
    Money ten = new TestMoney(10, "CUR");

    //Act
    Money actual = five.Plus(ten);

    //Assert
    actual.AsString().Should().Be("[amount=15][currency=CUR]");
}

We're still doing this in Money as it's where the behavior belongs. I feel this is a huge boost to productivity vs trying to do it in a subclass, then pull it up, and tweak things. The TestMoney object in our MoneyTests makes it easy to accomplish this.

While our test fails, it doesn't fail for the right reason. threw exception: System.NullReferenceException: We don't want to fail for an NPE. We need to update the code so our test fails for the right reason... the wrong value.

public Money Plus(Money addend)
{
    return this;
}

We'll just return itself. It's not gonna be right, but it will be a Money and we can see it fail for the asserted reason.

And we do Expected actual.AsString() to be "[amount=15][currency=CUR]" with a length of 25, but "[amount=5][currency=CUR]" has a length of 24, differs near "5][" (index 8).

Make Money

Let's make the test pass!

Except... how do I create a money? I can't have mutability. There's mutability reasons for this, but also; we don't want to change the amount of money each object represents. That... shudder gets bad. We strive for immutability; let's not abandon it so readily.

There's a couple options. One I've done for other things is an inner derived class that is used for this kinda "returned type". It's hacky

public abstract class Money
{
    private sealed class InnerMoney : Money
...

It works. It's not terribly messy. but it's always a hack. It's a code smell that says, "I know I have derived types, but there's not a correct structure".

I can create a chain of methods that pass in lambdas to enable generating the right type... Not even gonna show that.

I could... and it hurts me to say... use reflection to invoke the constructor of the type the object actually is. While it'd work in this, same currency case, it'd fail for mismatched currencies.

Honestly... I think the Knowledge Classes need to go away. It's... not playing nice with the design. We'll clean that up when we're green. 'Cause we not. We're red.

So, quick fix to get the test to pass... We'll create an inner class as shown above.

private sealed class HackMoney:Money
{
    public HackMoney(double amount, string currency) : base(amount, currency)
    {
    }
}

This is clearly a hack, just to get to red, so we can properly start to refactor things.

Our Plus now looks like

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

Which gets us to green!

In the commit for this, I'm calling it out as dangerous git commit -m "F!! Add hackMoney to get test passing"

While it's a very small commit; the introduction of the HackMoney is risky. We could reasonably keep it out until... ???? but... Small commits are better. We know what's going on, and it's clear why it exists.

But now we're green.

Now we can refactor

Make It Money

How do we go about removing the knowledge classes? The only place it makes sense for them to be is part of Money. Every behavior they have is in Money. While I use this type of Knowledge Class pretty regularly; I don't disagree that a behavior-less class is a class we shouldn't have. It's a conflicting principle with having a Single Point of Truth for things in the system, and Single Responsibility. If we're making Money know about how to construct a Dollar, and Euro, and ... Then it knows... and does... A LOT. I expect I'll have a strong urge later to refactor these things out, but they have to be there first. So let's move the Won.

While staring and looking to get SOMETHING set up, I did

public static Money Won(double amount)
{
    return new Won(amount);
}

And... I kinda like that. Now, Won itself needs some fixing.

internal sealed class Won : Money
{
    public Won(double amount) : base(amount, "KRW"){}
}

Making it internal will help prevent consumers of Money from accessing it directly.

But... Feels wrong. I like what it does... but ... the code doesn't feel right. I learned to trust the code. It's talking to me, it's saying this isn't ideal; and I have simpler ways... so I'll go with that

A change to Money that it's no longer abstract we can just create Money as

public static Money Won(double amount)
{
    return new Money(amount, "KRW");
}

An update to the Won Test

[TestMethod]
public void AsStringReturnsValueAndCurrency()
{
    //Arrange
    Money subject = Money.Won(9.7);

    //Act
    string actual = subject.AsString();

    //Assert
    actual.Should().Be("[amount=9.7][currency=KRW]");
}

shows that it still works. I'm good with this.

There's some klunky to it. It's got a smell to it. But less of a smell. The code doesn't cry out to me that it's wrong.

I'm pretty against static methods. I do not like them. It causes coupling between the consuming and implementing code. There are ways this isn't as bad. Languages that allow methods on interfaces (which... not a fan) make it not too bad since they are part of the interface. Ignoring language features, it's base types. Not base along inheritance hierarchy, but types in the system that are at the base. They don't derive from anything, they don't implement anything. Classes that tend to require access to the internal structure of the same type. It's easy to go, "Oh yeah, this totally needs it, guess it's a base type". No no. Base types FORCE themselves into the system. You either discover they are a base type, or you violate a bunch of development practices to prevent it from being a base type.

My mentor referred to this as, "Objects that churn against themselves". The object must know the details of the same type of objects it interacts with. Looking at our Plus method. We HAVE to get access to the variables of the Money addend otherwise we can't add them together.

We can't expose the _amount or _currency as those are primitive values that no one else needs to know. If those are needed for something, pretty sure Money can provide that functionality.

Hack It Out

Let's remove the HackMoney now that we can instantiate Money.

Euro Dollar

Let's do the Euro and Dollar as well into static methods as part of Money and get their tests updated.

Those are a pretty quick and easy change, and nice simple deletion.

public static Money Euro(double amount)
{
    return new Money(amount, "EUR");
}

public static Money Dollar(double amount)
{
    return new Money(amount, "USD");
}

An interesting thing now, all we have is Money. But we have 4 test classes. Since we're green, we should move these into Money. Fortunately it's not hard, it's one method each. Though a rename is in order.

[TestMethod]
public void DollarGeneratesWithUsdCurrency()
{
    //Arrange
    Money subject = Money.Dollar(5.1);

    //Act
    string actual = subject.AsString();

    //Assert
    actual.Should().Be("[amount=5.1][currency=USD]");
}

While The value is there, we don't really care about it. We just want to make sure the currency is the same.

We'll do the same move and rename for Won and Euro. Which let's us delete the currency specific test files.

And we're still green!

Refactoring?

We've been refactoring through a lot of this. But we were getting things done with those. It's really good to look through the code with out trying to accomplish things. What can we clean up? What's can be improved?

Our Money.cs file has an unused using. Often we can configure the IDE/Tools to take care of those for us. I've had some pain with it. I have it on about 1/2 my systems. My "active coding" projects and for work I definitely like having it on. The less I have to make an r Remove unused usings commit the happier I am. Otherwise everything clutters up and those commits end up modifying 10's to 100s of files. That's not good for anyone; Keep it clean, and it'll be better.
ReSharper makes it super easy to clean up a file, project, or solution .

I feel like Money is starting to have it's methods stabilized. We haven't touched most of them since they got moved into Money. They're all beautiful in their single-line-ness. Which allows us to make them expression bodies.
Going from

public double Times(int multiplicand)
{
    return _amount * multiplicand;
}

to

public double Times(int multiplicand) => _amount * multiplicand;

I much prefer expression body syntax. Let's refactor all the methods into that. Which our tooling does for us.

Times Money is Money

I guess... we should now make the Times method a Money returning method.

Seems like a good place for a test. Let's create a new method that does what we want. I think this is the better/cleaner/safer way to do it, but I'll also show the "change in place" approach, which is favored by one of my favorite colleagues to pair with.

And the steps for this are

• Copy/Paste existing Multiplication method
• Which won't compile due to name conflcit
• Change the Should().Be value to be a account/currency string
• Change the type of actual to Money
• Update the name

Resulting in this test

[TestMethod]
public void MoneyValueByMultiplicandShouldBeExpectedMoney()
{
    //Arrange
    Random random = new();
    int value = random.Next(1, 20);
    int multiplicand = random.Next(1, 20);
    Money subject = new TestMoney(value, "CUR");

    //Act
    Money actual = subject.TimesMoney(multiplicand);

    //Assert
    actual.Should().Be($"[amount={value * multiplicand}][currency=CUR]");
}

Which has the TimesMoney method not existing. Could I do the "Exists" style test... yes. Should I have... Yeah.... But ... Sometimes I don't. :| It's usually harder to stay consistent with that one when I can copy/paste an existing test to get the final form I want.

The simple initial implementation is

public Money TimesMoney(int multiplicand)
{
    return this;
}

Let's remember that we don't want to implement the code to pass the test until we see the test fail for the reason we want. Which... it did not Expected actual to be "[amount=27][currency=CUR]", but found MoneyExample.MoneyTests+TestMoney.

I would have discovered this if I did try to implement the final code, but... Checking the failure message showed me the failure for the wrong reason, and we should fix it to fail for the right reason.

We can definitely see it failed on the assert, but I expected just the amount to be different... not get an object description.

We'll change our assert to use the AsString method to get the value out we want.

actual.AsString().Should().Be($"[amount={value * multiplicand}][currency=CUR]");

And now it fails for the reason I expected Expected actual.AsString() to be "[amount=12][currency=CUR]" with a length of 25, but "[amount=1][currency=CUR]" has a length of 24, differs near "][c" (index 9).

To make this test pass, I'm going to do the simples bit possible - use existing code.

public Money TimesMoney(int multiplicand)
{
    return new Money(Times(multiplicand), _currency);
}

We already have the log for the multiplication, so let's just use what works.

Now that we have our multiplication that returns money, we can remove other method.

I like to think in the Test-Commit-Revert (TCR) approach. I COULD just start deleting stuff, but that will put us into a non-compiling state, and possibly a failing test. Which, with TCR, would revert and undo the delete. Gotta take tiny steps to keep the system in green.

First is to update our non-Money test to make sure we get the right stuff back.

I'm updating the //Act to be double actual = subject.TimesMoney(multiplicand).Times(1); This will use the new TimesMoney to do the work, but then Times to get the value the test compares against. We could do more updates to the test to not need this extra Times call, but - then we'd be testing something different.

It all works.... So, now we delete it.

Part of this is that we also need to remove the method, but it's used by our new method... Let's inline!

public Money TimesMoney(int multiplicand) => new(_amount * multiplicand, _currency);

Finally, we can rename this to be Times. And that's done.

Divide the Lewt

Let's do the Divide method the hard way.

• Change the return type to Money
  This breaks the body of the method... let's make that a Money
  There's a compilation issue in our tests now.
• Update the type in the //Act to Money
  Test compiles and fails... Money is not a double
• Change the ... Crap... Can't.

How do we confirm we didn't mess up some logic in the Divide method? Our test is going to change to be able to validate what may have been a mistake.

Nope. Don't like this at all. It's not safe. I don't know if anything works until the very end... AND that assume I didn't make any mistakes along the way.

I'm gonna do a shorter version than I did for Times, but still safe.

• Copy/Paste Divide Test
• Update Test to create DivideMoney
• Use simple to ensure failure for the right reason
• AND CHECK IT!

I'm gonna pause the list here and highlight that looking at the failure pointed out something new to me, that's probably a requirement. How many decimal places do we want our money to go?
Depending on the context it will vary. But for our current problem - how many?

I think it really matters, and can drive additional evolution of the code. I don't think I'll add it now. The problem domain appears very "financial institution" based... which deals with the many fractions of money.

• Update simple divide to use DivideMoney first
• validate still green
• Delete test
• Inline Divide method
• Rename DivideMoney to Divide

OK, done.

The simpler aspect is that I didn't commit during the process.
It felt dirty. Too many lines change.

It also introduces "questions". Did I add, or update the Divide method? Neither, both? Things stay really clear when you do micro-commits. We can literally watch the process play back.

Clean up

I let the tool generate code for me, it places the methods where it wants... normally at the end of the class. This means that we sometimes have to go clean up our class. Our tools do this FANTASTICALLY... if we configure them.

I don't have mine configured. ReSharper does it. The settings can also be saved to ensure the project and/or team always use the same set of code styles.

There's also the .editorConfig files that help IDEs know what type of rule to apply. I've used both successfully.

Standing Out

There's a couple bits in the Money class that stand out. The big one - It's not abstract.  That should get fixed.

Knowing what it will do, we can no longer use the TestMoney for our tests. We have to update to use one of the defined currencies.
Then we can make Money sealed.
Once we do that, ReSharper helpfully points out our constructor can now be made private. Let's do that.

This gets us very close to what we had before with the Knowledge Classes that prevents wonton creation of the Money type. We can only do so through controlled methods.

Another piece that stands out in Money is the methodAsString. What's this method here for? It's like we're using it to compare objects... Which... yeah... If we could actually look at the values, much simpler to validate.

We're very against returning the raw values; we don't want anyone to have access to those. For 2 very big reasons, which I may have mentioned;

• No Primitives should flow through our code.
• We don't want to reveal our internal values.

With AsString being used for test to compare values... what if we could create a money object of our expectation and then compare against the actual?

Something like

[TestMethod]
public void MoneyEqualsSameMoney()
{
    //Arrange
    Money subject = Money.Euro(12);

    //Act
    bool actual = subject.Equals(Money.Euro(12));

    //Assert
    actual.Should().BeTrue();
}

Since this fails... we know we're onto something!

We would get a passing test if we put subject.Equals(subject), but we know that does reference equality; and we don't want that.

Our tools will generate equality things for us. Let's hit those buttons and see what it looks like.

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

public override bool Equals(object obj)
{
    return ReferenceEquals(this, obj) || obj is Money other && Equals(other);
}

public override int GetHashCode()
{
    return HashCode.Combine(_amount, _currency);
}

These auto-generated methods make our test pass. I think they're good. They aren't good enough forever, but they get us to a point we can update our tests to use object equality instead of some string comparison.

With a passing test, we can go edit the rest of our tests to stop using AsString allowing us to delete the method.

Not all of the AsString consuming tests can be replaced. Some are checking the Currency value. Which... was useful.

What is it we really care about? Is it the currency; or that 2 currencies aren't the same? The second one sounds important to me.

This gets into what tests should be/do. If we change the currency of USD to usd... why should our test change? There's no new behavior. No behavior has changed AT ALL... and yet... tests would need to be updated... Clearly the tests are accomplishing their goal the wrong way. It's too tightly coupled to the implementation details of the currency information.

The tests that are still using AsString should be replaced by ones that use the Equals behavior. Or original Equals test using Euro's can replace the EuroGeneratesWithEurCurrency test.

We need to add another one those, that confirms non-equality.

[TestMethod]
public void EurosAndWonAreNotEqual()
{
    //Arrange
    Money subject = Money.Euro(12);
    Money other = Money.Won(12);

    //Act
    bool actual = subject.Equals(other);

    //Assert
    actual.Should().BeFalse();
}

And... it passes! YAy... wait a minute... Tests that pass the first time aren't useful tests...

True... ish. They aren't useful tests for TDD. We need some version of the non-equality test to ensure we don't regress and have all currencies equal. I think later changes will doubly confirm this, and maybe remove this explicitness...

I've specified both Euro and Won in this... but that'll create a exponential set of tests if we do every combination...

Or we randomize...

[TestMethod]
public void DifferentTypesAreNotEqual()
{
    //Arrange
    Random random = new();
    int value = random.Next(1, 20);
    Money euro = Money.Euro(value);
    Money won = Money.Won(value);
    Money usd = Money.Dollar(value);
    Random random = new();
    List<Money> shuffled = new List<Money> 
    {euro, won, usd}
    .OrderBy(a => random.Next())
        .ToList();


    //Act
    bool actual = shuffled[0].Equals(shuffled[1]);

    //Assert
    actual.Should().BeFalse();
}

and we have a nice simple place to ensure matches.

Instead of a method to make sure every currency is equal, we can apply randomization to keep it to a single method.

While it's may not catch everything perfectly, since we're running the tests a lot, and keeping everything clean... it should break pretty quick if either of these expectations are no longer true.

[TestMethod]
public void SameTypesAreEqual()
{
    //Arrange
    Random random = new();
    int value = random.Next(1, 20);
    List<Money> source = new() { Money.Euro(value), Money.Won(value), Money.Dollar(value) };
    List<Money> compare = new() { Money.Euro(value), Money.Won(value), Money.Dollar(value) };
    int index = random.Next(0, source.Count);

    //Act
    bool actual = source[index].Equals(compare[index]);

    //Assert
    actual.Should().BeTrue();
}

We don't have perfect coverage of the generated methods, but they're generated so we can feel some comfort in their correctness.

Now we can delete the remaining AsString tests and the method.

One more pass over the tests shows us some more we can clean up.

Next Time

I think this calls our current requirement DONE!

5 USD * 2 = 10 USD
10 EUR * 2 = 20 EUR
4002 KRW / 4 = 1000.5 KRW
5 USD + 10 USD = 15 USD
5 USD + 10 EUR = 17 USD
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD

The next thing we'll tackle is adding different currencies together.