Extend the TestCurrencyConverter #3249
Conversation
Just for completeness ...
Make it possible to support multiple currencies. Plus some simplification.
| @@ -30,23 +31,43 @@ public class TestCurrencyConverter implements CurrencyConverter | |||
| EUR_USD.addRate(new ExchangeRate(LocalDate.parse("2015-01-09"), BigDecimal.valueOf(1.1813).setScale(10))); | |||
|
|
|||
| EUR_USD.addRate(new ExchangeRate(LocalDate.parse("2015-01-12"), BigDecimal.valueOf(1.1804).setScale(10))); | |||
| EUR_USD.addRate(new ExchangeRate(LocalDate.parse("2015-01-13"), new BigDecimal("1.1782"))); | |||
There was a problem hiding this comment.
That day was missing for whatever reason. Calling the BigDecimal constructor with a string will ensure that the value is exact.
| @@ -55,31 +76,24 @@ public String getTermCurrency() | |||
| return termCurrency; | |||
| } | |||
|
|
|||
| @Override | |||
| public Money convert(LocalDate date, Money amount) | |||
There was a problem hiding this comment.
We can leave this to the superclass. It is enough to override getRate().
There was a problem hiding this comment.
It is delegating to the super class, but "just" converts them 1:1 for any currency conversation that are not part of the test currency converter. Has this now become obsolete by introducing CHF explicitly?
There was a problem hiding this comment.
I don’t understand the remark (or question?). The behaviour (1:1 conversion for unknown currencies) is exactly the same as before:
- Currently (before this change), it is done in this function,
convert(), in line 69, by creating a newMoneywith the desired currency and the existing amount. - With this change,
convert()in the superclass calls ourgetRate()to find the exchange rate, andgetRate()returns 1 for unknown currency pairs. The superclassconvert()uses this rate for the conversion, with the same effect.
I don’t actually think any tests rely on this behaviour, though.
There was a problem hiding this comment.
You are right. I forgot (and was looking at the PR only in the browser without full access to the code)
I added the fallback conversation rate later, which makes the overriding of getRate obsolete. Agree.
|
I wrote this some time ago because I am going to need it (inverse conversion, and second foreign currency) to test some other change that I hope to finish some other day. I’m submitting it now because #3248 is also trying to add inverse conversion to the |
| { | ||
| // testing: any other currency will be converted 1:1 | ||
| if (!amount.getCurrencyCode().equals(series.getBaseCurrency())) | ||
| return Money.of(termCurrency, amount.getAmount()); |
There was a problem hiding this comment.
Just to point it out, this is where the 1:1 conversion happens in the old code …
| series = EUR_CHF; | ||
| else | ||
| // testing: any other currency will be converted 1:1 | ||
| return new ExchangeRate(date, BigDecimal.ONE); |
There was a problem hiding this comment.
… and this is where, in the new code, an exchange rate of 1 is passed to the superclass’s convert(), which uses it to do a 1:1 conversion.
(Edit: Just saw that it is technically not a superclass, but an interface with a default implementation. Doesn’t make a difference in the end, however.)
Extend the
TestCurrencyConverterto support EUR↔USD and EUR↔CHF (either direction; not USD↔CHF, though). Also some refactoring to make it simpler.