Make Money with TDD - 05
Cross Currency
Our next requirement is to add different currencies together.
5 USD * 2 = 10 USD10 EUR * 2 = 20 EUR4002 KRW / 4 = 1000.5 KRW5 USD + 10 USD = 15 USD
5 USD + 10 EUR = 17 USD
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD
I'm really glad I included the requirement to add the same currency together. It's a very nice, small, simple step that actually generated a lot of refactoring to the code based on what came up.
I'd probably have gotten to a very similar point if the effort was to add different currencies; but... I may not have. There would be another whole set of code to handle the currency interaction that may have cluttered up the code and made it harder to see the improvements that could have been made.
Small Steps. Quick Feedback. These things are huge when developing high quality software, quickly. While it seems "silly"... Going those and methodical allows us to see things and improve the code the enables us to go faster as we continue to make those smalls steps in the code.
Go slow to go fast
Mix It
Anyway; onto multiple currencies!!!
This should be an obvious test, using the requirement provided.
[TestMethod]
public void FiveDollarsPlusTenEuroShouldBeSeventeenDollars()
{
//Arrange
Money fiveDollar = Money.Dollar(5);
Money tenEuro = Money.Euro(10);
//Act
Money actual = fiveDollar.Plus(tenEuro);
//Assert
actual.Should().Be(Money.Dollar(17));
}
And it fails! Is it the right reason?
Expected actual to be MoneyExample.Money{}, but found MoneyExample.Money{}.
Ummm... I... don't really know...
That's clearly the output from FluentAssertions using the ToString method. The one place I'm comfortable using ToString in classes is for test output clarity. Even for debugging I prefer other techniques, usually DebuggerDisplayAttribute.
So, we'll add public override string ToString() => $"{_amount} {_currency}"; to Money so that we can have informative test output.
The failure now tells us exactly what we want to see.Expected actual to be 17 USD, but found 15 USD.
As should be expected, it just ads the values together and the left hand side currency.
This little change is a great example of why I feel it's CRITICALLY important to a code base that the tests have informative output. Even if you, as the developer, know what the error message means, you might forget, or someone else might have to work in the code. If there's any question about what the test failure message means, go figure out how to improve it to remove the question. If you have to go look at the code to deduce the meaning, change the code to make the meaning obvious.
In this example; before the ToString method, we'd have to debug the test with a breakpoint set. Then inspect both the result to see what the values are.
OR - We'd be inclined to expose the private class variables so we could interrogate and report on them individually... Terrible options to maintain a clean code base.
The easier it is to diagnose a test failure, the faster it is to fix a failing test. Anyone that has to touch the code in an emergency will thank you.
And it'll probably be you.
Pass It
OK, let's get to green so we can see what we can clean up!
I waited so long to convert the methods to expression bodies as I thought they were gonna be pretty stable for a bit... now I need to revert Plus to be a Statement body to make it easier to work in.
I have a very strong inclination to make the code base be as easy to work in as possible. Expression bodies are great for reading code. Not as great when you want to edit a It's why I use them as a mental tracker to indicate I expect the method to be pretty stable. Which is was... until we added a new requirement.
Always feel comfortable to change the shape of the code to make it the easiest to work in. Once the work is done, it can be re-shaped to a easier to read form.
A quick, simple implementation to get to green.
public Money Plus(Money addend)
{
double conversion = 1.2;
return new(this._amount + addend._amount * conversion, _currency);
}
100% makes the test pass! OH YEAH!
I did quick math to get the conversion rate. We want 5 USD + 10 EUR = 17USD and there's a piece missing here. There has to be a conversion of EUR to USD. While we, the sharp developers, understand that, it's not actually represented in the equation. If we represent the conversion in the equation, we'll be able to re-arrange things and figure out the conversion rate.
5 USD + 10 EUR * X = 17 USD
10 EUR * X = 12 USD
X = 12 USD / 10 EUR
X = 1.2 USD / EUR
I'd like to thank Dr. Brad Johnson for the draconian method of our homework in Quantum Mechanics that had us align the equals sign in all of our equations. It's stuck with me ever since.
One thing that stands out, a bit painfully, to me now is that the conversion is not just a value, it's a ConversionRate. the 1.2 only applies when we want USD and have EUR.
My simple implementation did get the failing test to pass... but another test fails! Our original addition of 5 EUR + 10 EUR = 15 EUR now equals 17 EU Which... clearly no longer the right answer.
What do we do?
We have a red test, we fix it - right?
Ish.
We broke a test "unexpectedly" with our new code. We don't try to fix the newly broken test, we revert our changes. We want our existing functionality to stay green. If we keep chasing changing red tests, we're going to add complexity into the system. We'll no longer have a "good state". We'll have to keep adding and editing code. I'm scared of that world.
Let's undo our addition of conversion. We need to have something in place that recognizes the type we want to go to and from.
Strategic Withdrawl
Alrighty, once we've reverted our code, let's see what we can do about making it A LITTLE smarter... but still really simple and hacky.
My favorite way to add code to make the test pass is to identify the conditions of the test, and put an if block for those conditions to add the code to make the test pass. I didn't do that in my original test, which allowed another test to break. That was a lazy mistake on my part.
Even in super simple code, adding the conditional clause for the test you're working in makes it so you can roll back QUICK and SAFELY.
The safely is the big part.
While I had to modify the existing code a little, I have set it up so that all the changes for the red test are in the if block
public Money Plus(Money addend)
{
double otherValue = addend._amount;
if (this._currency == "USD" && addend._currency == "EUR")
{
otherValue *= 1.2;
}
return new(this._amount + otherValue, _currency);
}
I could comment/delete the entire if block, and we'd be back into a known good state. I used the tools to extract addend._amount into a variable, so I know that was safe. The if block contains all the customized code for the test to pass.
Which it does now; putting us into the green state, which allows us to Refactor!
To Triangulate or Not to Triangulate
What's there to refactor? There's no pattern in the code yet. The code isn't telling me what it needs to be better. I could add another test, and I probably should, to force a pattern into the code. I wonder if I can update the existing code to draw out an assumption in the code. The return new Money(_amount otherValue, _currency);assumes the same currency. Let's make it explicit.
Ooo.... Looking at the code, I think I extracted the wrong thing. Let's not extract the _amount, but create a variable for the exchange rate.
Updating the method to be
public Money Plus(Money addend)
{
double exchangeRate = 1.0;
if (this._currency == "USD" && addend._currency == "EUR")
{
exchangeRate = 1.2;
}
return new(this._amount + (addend._amount * exchangeRate), _currency);
}
Keeps us green and shifts the mutable value from the amount to sum, to be an exchange rate. Same basic flow, but I like the exchange rate being the value getting updated.
I feel like it's safer. If we screw up and it hits 2 branches, the amount added doesn't get multiplied twice, the exchange rate just gets re-set. Feels safer that way.
Now, about that assumption in the code. It's represented in the updated method with the exchange rate being set to 1.0. Why is it one? Because equal currencies don't change their value.
Let's extract that assumption into explicit code.
public Money Plus(Money addend)
{
double exchangeRate = 0.0;
if (this._currency == "USD" && addend._currency == "EUR")
{
exchangeRate = 1.2;
}
if (this._currency == addend._currency)
{
exchangeRate = 1.0;
}
return new(this._amount + (addend._amount * exchangeRate), _currency);
}
Now we are being very explicit that equal currencies don't change. I like this a little... but it definitely does not clearly give us the pattern we know we'll have to implement. I need something else with the && element to look the same. Well... Let's go ahead and be even MORE specific...
public Money Plus(Money addend)
{
double exchangeRate = 0.0;
if (this._currency == "USD" && addend._currency == "EUR")
{
exchangeRate = 1.2;
}
if (this._currency == "USD" && addend._currency == "USD")
{
exchangeRate = 1.0;
}
if (this._currency == "EUR" && addend._currency == "EUR")
{
exchangeRate = 1.0;
}
if (this._currency == "KRW" && addend._currency == "KRW")
{
exchangeRate = 1.0;
}
return new(this._amount + (addend._amount * exchangeRate), _currency);
}
Now I have my identical-ness of all the exchange rates we use. Clearly there's some pattern here.
One thing that is a bit more obvious here as a C# stylistic thing; I use this sometimes. There's a reason to the variance. I am generally opposed to using this prefixing class variables. I have the IDE/Tools recommend that it get deleted, so it's a dark gray color telling me I should delete it. Most of the time, absolutely.
So... Why keep it here?
Make similar things more similar.
We want to have similarities amplified. Accessing the _currency variable from the addend we MUST have the object prefix. To make the code even more similar, we also use the object prefix for the object we're in... this.
Cartography Time
Clearly we need some kind of mapping of currencies and their exchange rate. Since we're still green, and have a pretty explicit pattern in place... Time to hack some code into place.
First, let's clean up where we're gonna work - Extract the conversion logic into it's own method. It's not part of the Plus method, but something that the method interacts with... Almost like I think it'll become it's own object... DUH DUH DUNNNNNNN!
public Money Plus(Money addend)
{
double exchangeRate = ExchangeRate(this._currency, addend._currency);
return new(this._amount + (addend._amount * exchangeRate), _currency);
}
private static double ExchangeRate(string toCurrency, string fromCurrency)
{
double exchangeRate = 1.0;
if (toCurrency == "USD" && fromCurrency == "EUR")
{
exchangeRate = 1.2;
}
if (toCurrency == "USD" && fromCurrency == "USD")
{
exchangeRate = 1.0;
}
if (toCurrency == "EUR" && fromCurrency == "EUR")
{
exchangeRate = 1.0;
}
if (toCurrency == "KRW" && fromCurrency == "KRW")
{
exchangeRate = 1.0;
}
return exchangeRate;
}
You'll probably notice a couple other changes to the if conditions. Mostly, it no longer deals with objects. I did this to preserve the similarity in interaction.
We're also a lot more explicit about the conversion direction.
The tooling has made the ExchangeRate method static since it doesn't depend on any of the class instance information. I don't like static methods. This one's private, so I care less. It's also a great indicator that the method is decoupled from the object itself, clearly. This tells us where we might want to extract another object. If we'd used the this reference in the method, it'd be tightly coupled, and take more work later on to extract, if that's what happens. (Spoiler: it will be)
Letting it be tightly coupled means that it could become MORE tightly coupled as the code evolves. We want to avoid coupling where we don't need it, and we don't here.
OK, let's refactor this into... something?
Let's sketch out some ideas. I don't know the answer. The one I WANT to do... doesn't feel like I can force it in here. I mean, I COULD. It's not obvious though. It's not what the code is asking for right now. It wants to transform what it has into something less... ify.
We're mapping a value to another value and that indicates a value to return. We could construct a pair EUR->USD and have that associated with 1.2. I'm not a fan of that. We're having to construct a string to represent someting.
We could do nested ifs... which we can discount right away, that's almost never a good option. I had to use nested case statements once. Not the most maintainable, but definitely the most clear; and our fancy maps didn't work.
We could do nested case... but that's not maintainable... that's not Object Oriented.
We're going to have a collection... of a value that ties to another value resulting in a value. A map of a map.
... Oh geez... what's C#'s map type... I forget this all the time... Have to look it up all the time. It's a Dictionary.Why can't it just be a Map?
Anyway... Here's a Dictionary-Dictionary
private static double ExchangeRate(string toCurrency, string fromCurrency)
{
Dictionary<string, Dictionary<string, double>> map = new()
{
{ "USD", new() {{ "USD", 1.0 }} },
{ "KRW", new() {{ "KRW", 1.0 }} },
{ "EUR", new()
{
{ "EUR", 1.0 },
{ "USD", 1.2 }
} }
};
double exchangeRate = 0.0;
if (map.ContainsKey(fromCurrency) && map[fromCurrency].ContainsKey(toCurrency))
{
exchangeRate = map[fromCurrency][toCurrency];
}
return exchangeRate;
}
This keeps us green!
We obviously don't want to rebuild the map every time; That's what refactoring is for!
A question here is - what if there's no exchange rate? What do we do?
Currently that'll result in 0 of whatever the toCurrency is. Is this the right result? It feels pretty wrong. it's going from some value to no value. Not only is the result confusion, it's impossible to tell the difference between a conversion of 0 to 0 and a non-conversion result. We can't have that. Indeterminate results is not a good way to return results. One thing I kinda like about GoLang is the error response result. While we can duplicate that in C#... it's clumsy and I don't like it getting hacked in like that.
Should we return a result? Or should we throw an exception? Between the two, and without a more exact specification, I think throwing an exception is the better option here.
I generally avoid throwing exceptions. If we know the situation can occur, such as no exchange rate available, then we should build the system to provide that feedback without using an undocumented goto. I've kinda convinced myself to spend a little more time figuring out what other options might exist... I don't know if I'll find one. We're dealing with a very narrow scope right now - Build the Money object with some behaviors. How is the object going to be consumed?
That's not defined. And that's the type of information which really helps determine what a good non-exception communication pattern looks like. My default would be some form of the NullObject Pattern. A NullMoney. But... How's that used? What should it do? Since I don't know what the system will do when it handles an exception, I can't determine what behaviors NoAvailableConversionMoney should do. But... I can still create it. See... The code is crying out for a way to represent "No Conversion Available" and instead of going, "No idea - Catch!" we sit and think... and use the patterns and practices we know make code better. What will our NoAvailableConversionMoney type do?... It'll throw exceptions. We don't know what it should do, because that'll be highly dependent on the system it's being consumed in.
This, of course, assumes it's part of the system and not being consumed as a 3rd party library. If I'm am building this as a library, I'd probably throw the exception. Again - Really depends on the broader context. I'd try to not throw an exception, sometimes, at least for libraries, I think it's the only choice.
No Conversion
Let's get a test in place to validate the lack of a conversion rate gives us back a Null Object. Which... Might be hard when Money is ... sealed. Some languages have a feature that you can extend a sealed/final class as long as it's being done in the same file. ... Wish I had that. Anyway, I've been in C# long enough, I have work arounds, like private constructor; which we already have.
Here is the test method to help us create the object
[TestMethod]
public void NoConversionResultsInNullObjectType()
{
//Arrange
Money realCurrency = Money.Dollar(5);
Money fakeCurrency = Money.FakeCurrency(10);
//Act
Money actual = realCurrency.Plus(fakeCurrency);
//Assert
actual.Should().BeOfType<NoAvailableConversionMoney>();
}
Should I do the Exists style tests... Yeah, I should. It does follow the explicitness much better. I do find that I start to drop doing them when it's extending existing things. New objects, always exists; extending behaviors with a new object... ehhh.... it's much less clear cut for me. I'll think about it, and... if I think about it too late, such as after I've already created the test case for what I want to see happen... I'll solace myself with the fact I still thought about it.
It's kinda crappy that I'm not doing the Exists test, as this test introduces two concepts; the FakeCurrency static method and the typeNoAvailableConversionMoney.
Fine - you talked me into it.
[TestMethod]
public void Exists()
{
Money exists = new NoAvailableConversionMoney();
}
And the type does not exist, so we're ready to make it!
I have it as
public sealed class NoAvailableConversionMoney : Money
{
public NoAvailableConversionMoney() : base(0, "No Conversion") { }
}
... but I'm not happy with it. I don't think there's enough information to do this as an effective pattern yet. At least there's not for me.
Let's go back to the idea of throwing an exception. While I don't like it... that's actually a positive. It'll stand out as something to improve... I just don't know how to improve it yet in a way the code likes.
Listen to the code! Just because we want to see something, doesn't mean it's the right thing.
If there's no conversion, we'll throw an exception.
[TestMethod]
public void NoConversionResultsInNullObjectType()
{
//Arrange
Money realCurrency = Money.Dollar(5);
Money fakeCurrency = Money.FakeCurrency(10);
//Act
Func<Money> actual = realCurrency.Invoking(x => x.Plus(fakeCurrency));
//Assert
actual.Should().ThrowExactly<NoExchangeRateAvailableExceptio>();
}
The biggest change here is the body of the ExchangeRate method
if (!map.ContainsKey(fromCurrency) || !map[fromCurrency].ContainsKey(toCurrency)) throw new NoExchangeRateAvailableException();
return map[fromCurrency][toCurrency];
Theres an interesting bit for the FakeCurrency method - It's internal
internal static Money FakeCurrency(double amount) => new(amount, "Not A Real Currency");
I often get a lot of pushback for putting "test" code into "production" code. While I have Money locked down; this is the best way. How Money types get constructed might evolve in a way that the FakeCurrency can move to the testing space.
Not sure yet.
What we don't want to do is sacrifice the design of our system so we can do things for tests. There's currently some test focused code that is adhering to the design of the system. This is far more important to me, which means I find it far less of a smell, than leaving Money non-sealed so that we can happen to create something in a test class.
To be fully transparent here - I wouldn't actually do what I have either. I've developed some utility classes that use reflection and can construct objects via private constructors. These are TEST ONLY classes, but then enable me to do what I need in the tests, while preserving the design of the system. I'm not opposed to having code that exists solely to test with; but it's smell and I like alternatives. If we have something we consider a good design but it can't be effectively tested; it's a bad design. If we can't test it, we can't trust it. If we can't trust it, it's a bad design.
I'm not including my reflection-based utility classes as part of this exercise. I'll link to/include them at the end. For now, lets accept a little smell so we can ensure behavior.
Exchange Elsewhere
The last piece to call our current requirement done is to extract the exchange rate out. Money should not know the exchange rate into. It's job is to know about a value and a currency. Something else knows how two currencies relate. We need to create that something else.
Do we need tests? That's a good question. I think we can generate the new class fully through tool refactorings.
I'm gonna try to do that, and we'll see how it goes.
I couldn't see how to make the method non-static via the tool. I'm sure it's there, but I had to manually delete it. ...
OK, that sucked. Could I? ... not entirely. And it'd take a bit of putzing around. I'm currently using Rider IDE (from JetBrains). I started on (and wish I was currently using) Visual Studio. I'm more familiar and would be more comfortable pushing through to see how it goes. My lack of familiarity with the tool makes it less comfortable to try and experiment with the refactorings while writing this as well. So... I'm just gonna manually extract it. I dislike this approach as it is higher risk. The tools are better/faster at most things, and I prefer to use them. There's a few "extract to new class" flows that don't go as clean as I'd like; so I fall back to manual.
One reason I avoid manual is that it tends to break everything until you're done. That's gonna be some time in a broken state. When using the tools to do each step, you're green all the way.
A colleague much prefers the "break it" method. That way the compiler is always telling him what to fix up next. It's a different approach to using the tools, and I like it at times. I just wish I could hot-key what I'm trying to do, it'd be faster/safer.
ONTO THE SLOW AND DANGEROUS!!!
That wasn't too bad... but always feel.. gross. We're still green. The new ExchangeRates class is
public class ExchangeRates : IExchangeRates
{
private static Dictionary<string, Dictionary<string, double>> _map;
public ExchangeRates():this(StaticMap())
{}
private ExchangeRates(Dictionary<string,Dictionary<string,double>> map)
{
_map = map;
}
private static Dictionary<string, Dictionary<string, double>> StaticMap()
{
return new Dictionary<string, Dictionary<string, double>> ()
{
{ "USD", new() { { "USD", 1.0 } } },
{ "KRW", new() { { "KRW", 1.0 } } },
{
"EUR", new()
{
{ "EUR", 1.0 },
{ "USD", 1.2 }
}
}
};
}
public double ExchangeRate(string toCurrency, string fromCurrency)
{
if (!_map.ContainsKey(fromCurrency) || !_map[fromCurrency].ContainsKey(toCurrency))
throw new NoExchangeRateAvailableException();
return _map[fromCurrency][toCurrency];
}
}
This is set up to be able to take a custom set of exchanges, though not utilized yet. Mostly because I don't have my PrivateCtor class pulled in to access private constructors.
What this does give us though - We no longer need the FakeCurrency. Money no longer controls the exception being thrown. We need to change test to go against ExchangeRates and validate the exception there.
This is why I'm OK with small, obvious code smells. It helps us move along, and the code very clearly lets us know when we no longer need it.
Since we are having tests against another class... we should move it out.
[TestClass]
public class ExchangeRatesTests
{
[TestMethod]
public void NoExchangeRateIsException()
{
//Arrange
ExchangeRates subject = new ExchangeRates();
//Act
Func<double> actual = subject.Invoking(x => x.ExchangeRate("USD", "BLEEP_BLOOP"));
//Assert
actual.Should().ThrowExactly<NoExchangeRateAvailableException>();
}
}
As we no longer need the FakeCurrency Let's be sure to get rid of it.
What else moves?
Are there any more tests that are actually ExchangeRateTests?
There's one that feels like it could be
[TestMethod]
public void FiveDollarsPlusTenEuroShouldBeSeventeenDollars()
{
//Arrange
Money fiveDollar = Money.Dollar(5);
Money tenEuro = Money.Euro(10);
//Act
Money actual = fiveDollar.Plus(tenEuro);
//Assert
actual.Should().Be(Money.Dollar(17));
}
What is this doing? It's validating the exchange amount is correct. Which... That is very much the responsibility of the ExchangeRatesTests. This test isn't without value in the MoneyTests though.
It's confirming that the Plus method uses the exchange rate. What it is... that's not really important. We could definitely fake the exchange rate... which since I don't have my utility, a little harder to do without changing the visibility of the constructors, and I don't wanna.
I think I'll bring in my utility class at the end and show how we can test differently (I think more effectively) using it.
For now; I'll leave this test. It's accurate enough, but will break if we ever get a different exchange rate. It definitely needs to be kept in mind.
Wrapping Up
This definitely puts us at a good place to consider our latest requirement complete
5 USD * 2 = 10 USD10 EUR * 2 = 20 EUR4002 KRW / 4 = 1000.5 KRW5 USD + 10 USD = 15 USD5 USD + 10 EUR = 17 USD
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD
Equality isn't going to work though... Should it work? Do we want 1 EUR to be considered equal to 1.2 USD? I think so. For our purposes they are equivalent.
Let's add that as a requirement.
5 USD * 2 = 10 USD10 EUR * 2 = 20 EUR4002 KRW / 4 = 1000.5 KRW5 USD + 10 USD = 15 USD5 USD + 10 EUR = 17 USD
12 USD == 10 EUR
1 USD + 1100 KRW = 2200 KRW
2.50 USD * 6 = 15 USD
There we go.
Looking at this; pretty sure we can mark off the last requirement too. I thought it'd get resolved early on... but we know it is now. We even have a test that shows it...
OH ... OH... We don't! All of the tests that had an explicitness on being a double got changed to ints as random values. Guess we'll need to add a test for it... Can't call it complete yet!
But we'll consider that for next time. It is at the end of the list... so maybe not until then.
The requirement to add multiple currencies; that's definitely done.