In my last post I discussed the refactoring of a particular piece of code. Incrementally changing the code had resulted in some clear improvements in its complexity, but the end-result still left me with a unsatisfied feeling: I had not been test-driving my changes, and that was noticeable in the resulting code!
So, as promised, here we are to re-examine the code as we have it, and see if when we start testing it more thoroughly. In my feeble defence, I’d like to mention again why I delayed testing. I really didn’t have a good feel of the intended functionality, and because of that decided to test later, when I hoped I would have a better idea of what the code is supposed to do. That moment is now.
Again a fairly long post, so it’s hidden behind the ‘read more’ link, sorry!
The state of the code where we left it is below:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
|
package com.example.confirmationletter;
import java.math.BigDecimal;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import com.example.domain.Client;
import com.example.domain.Currency;
import com.example.domain.GenericRecord;
import com.example.record.service.impl.Constants;
public class ConfirmationLetterTotalsCalculator {
private List<String> currencies = Arrays.asList(Constants.CURRENCY_EURO,
Constants.CURRENCY_FL, Constants.CURRENCY_USD);
private class CreditDebitHolder {
private Map<String, Map<String, BigDecimal>> values = new HashMap<String, Map<String, BigDecimal>>();
public CreditDebitHolder() {
values.put(Constants.DEBIT, new HashMap<String, BigDecimal>());
values.put(Constants.CREDIT, new HashMap<String, BigDecimal>());
}
public BigDecimal getValue(String sign, String currency) {
BigDecimal value = values.get(sign).get(currency);
if (value == null) {
value = BigDecimal.ZERO;
}
return value;
}
public void setValue(String currency, String sign, BigDecimal value) {
values.get(sign).put(currency, value);
}
}
private class RecordFilterStrategy {
boolean filter(GenericRecord record) {
return true;
}
}
private RecordFilterStrategy sansAmountsFilter = new RecordFilterStrategy();
private RecordFilterStrategy faultyAmountsFilter = new RecordFilterStrategy();
private RecordFilterStrategy recordAmountsFilter = new RecordFilterStrategy() {
boolean filter(GenericRecord record) {
return record.isCounterTransferRecord() && !record.hasFee();
}
};
private RecordFilterStrategy balancedFilter = new RecordFilterStrategy() {
boolean filter(GenericRecord record) {
return !record.hasFee() && record.isDebitRecord();
}
};
private CreditDebitHolder recordAmounts = new CreditDebitHolder();
private CreditDebitHolder sansAmounts = new CreditDebitHolder();
private CreditDebitHolder faultyAccountRecordAmounts = new CreditDebitHolder();
private CreditDebitHolder balancedAmounts = new CreditDebitHolder();
private Utils utils;
private Client client;
/**
* Main entrypoint for this class. Calculates the Debit and Credit totals for
* the passed in records, faultyAccountNumberRecords and sansDuplicatesFaultRecords.
* <p>
*
* @param client The Client for which these are the records, used to get
* some default values from.
* @param records
* @param faultyAccountNumberRecordList
* @param sansDuplicateFaultRecordsList
* @return a map with currency as key, containing all calculated totals.
*/
public Map<String, BigDecimal> calculateRetrievedAmounts(
List<GenericRecord> records,
List<GenericRecord> faultyAccountNumberRecordList,
List<GenericRecord> sansDuplicateFaultRecordsList,
Client client) {
this.client = client;
if (client.isBalanced()) {
calculateTotalsOverRecords(records, balancedAmounts, balancedFilter);
} else {
calculateTotalsOverRecords(records, recordAmounts, recordAmountsFilter);
calculateTotalsOverRecords(sansDuplicateFaultRecordsList, sansAmounts, sansAmountsFilter);
calculateTotalsOverRecords(faultyAccountNumberRecordList, faultyAccountRecordAmounts, faultyAmountsFilter);
}
return calculateOverallTotalsForAllCurrencies();
}
private void calculateTotalsOverRecords(List<GenericRecord> records,
CreditDebitHolder amountsHolder,
RecordFilterStrategy filterCommand) {
for (GenericRecord record : records) {
if (filterCommand.filter(record)) {
addAmountToSignedTotal(record, amountsHolder);
}
}
}
private void addAmountToSignedTotal(GenericRecord record,
CreditDebitHolder amountsHolder) {
setRecordSignToClientSignIfUnset(record);
setRecordCurrencyCodeToClientIfUnset(record);
String currency = Currency.getCurrencyByCode(record.getCurrencyNumericCode());
amountsHolder.setValue(currency, record.getSign(),
amountsHolder.getValue(currency, record.getSign()).add(record.getAmountAsBigDecimal()));
}
private Map<String, BigDecimal> calculateOverallTotalsForAllCurrencies() {
HashMap<String, BigDecimal> recordAmount = new HashMap<String, BigDecimal>();
for (String currency : currencies) {
recordAmount.put(currency, calculateOverallTotal(currency));
}
return recordAmount;
}
private BigDecimal calculateOverallTotal(String currency) {
return calculateOverAllTotalForSign(currency, Constants.CREDIT)
.subtract(calculateOverAllTotalForSign(currency, Constants.DEBIT)).abs();
}
private BigDecimal calculateOverAllTotalForSign(String currency, String sign) {
return balancedAmounts.getValue(currency, sign)
.add(recordAmounts.getValue(currency, sign)
.add(sansAmounts.getValue(currency, sign))
.subtract(faultyAccountRecordAmounts.getValue(currency, sign)));
}
private void setRecordCurrencyCodeToClientIfUnset(GenericRecord sansDupRec) {
Integer currencyCode = sansDupRec.getCurrencyNumericCode();
if (currencyCode == null) {
Currency currency = utils.getDefaultCurrencyForClient(client);
sansDupRec.setCurrencyNumericCode(currency.getCode());
}
}
private void setRecordSignToClientSignIfUnset(GenericRecord tempRecord) {
if (tempRecord.getSign() == null) {
String sign = client.getCreditDebit();
tempRecord.setSign(sign);
}
}
}
|
Some initial tests
That code looks reasonable nice. But since I try to be somewhat professional in code, I needed to test, even if it was after the fact. I started out testing a few nicely self-contained methods, which I won’t discuss in detail, since they’re boring.
You can read those initial tests below, if you like. Then after those, I started to try to test the actual functionality of calculations and quickly ran into irritatingly convoluted test syndrome. The first is right below, in the non-collapsed code fragment.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
|
package com.example.confirmationletter;
import java.math.BigDecimal;
import junit.framework.TestCase;
import com.example.domain.Client;
import com.example.domain.Currency;
import com.example.domain.GenericRecord;
import com.example.domain.Record;
import com.example.record.service.impl.Constants;
public class ConfirmationLetterTotalsCalculatorTest extends TestCase {
ConfirmationLetterTotalsCalculator calculator = null;
Client client = null;
GenericRecord record = null;
@Override
protected void setUp() throws Exception {
super.setUp();
calculator = new ConfirmationLetterTotalsCalculator();
client = new Client();
client.setCreditDebit(Constants.CREDIT);
calculator.setClient(client);
record = new Record();
}
// Tests for defaults
public void testRecordSignSetToClientSignIfUnset() {
record.setSign(null);
calculator.setRecordSignToClientSignIfUnset(record);
assertEquals(Constants.CREDIT, record.getSign());
}
public void testRecordSignSetToClientSignIfAlreadySet() {
record.setSign(Constants.DEBIT);
calculator.setRecordSignToClientSignIfUnset(record);
assertEquals(Constants.DEBIT, record.getSign());
}
public void testRecordCurrencyCodeSetToClientCurrencyIfUnset() {
calculator.setUtils(createUtilsToReturn(Constants.EUR_CURRENCY_CODE));
record.setCurrencyNumericCode(null);
calculator.setRecordCurrencyCodeToClientIfUnset(record);
assertEquals(Constants.EUR_CURRENCY_CODE, record.getCurrencyNumericCode());
}
public void testRecordCurrencyCodeSetToClientCurrencyIfAlreadySet() {
calculator.setUtils(createUtilsToReturn(Constants.EUR_CURRENCY_CODE));
record.setCurrencyNumericCode(Constants.USD_CURRENCY_CODE);
calculator.setRecordCurrencyCodeToClientIfUnset(record);
assertEquals(Constants.USD_CURRENCY_CODE, record.getCurrencyNumericCode());
}
private Utils createUtilsToReturn(Integer currencyCode) {
MyUtils utils = new MyUtils();
utils.setCurrencyCode(currencyCode);
return utils;
}
class MyUtils extends Utils {
private Integer currencyCode;
public void setCurrencyCode(Integer currencyCode) {
this.currencyCode = currencyCode;
}
public Currency getDefaultCurrencyForClient(Client client) {
Currency currency = new Currency();
currency.setCode(currencyCode);
return currency;
}
};
}
|
Testing the calculations
1
2
3
4
5
6
7
8
9
10
11
12
13
14
|
// Tests for calculations
public void testCalculateTotalForCurrencyAndSign() {
calculator.balancedAmounts.setValue
(Constants.CURRENCY_EURO, Constants.DEBIT, BigDecimal.ONE);
calculator.recordAmounts.setValue
(Constants.CURRENCY_EURO, Constants.DEBIT, BigDecimal.TEN);
calculator.sansAmounts.setValue
(Constants.CURRENCY_EURO, Constants.DEBIT, BigDecimal.ONE);
calculator.faultyAccountRecordAmounts.setValue
(Constants.CURRENCY_EURO, Constants.DEBIT, BigDecimal.ONE);
BigDecimal result
= calculator.calculateOverAllTotalForSign(Constants.CURRENCY_EURO, Constants.DEBIT);
assertEquals(new BigDecimal(11), result);
}
|
Before we go into the ways in which this is not a very pleasant way to do testing, let me say that even this one simple test revealed how my refactorings from the precious post introduced bugs! In fact, and more painfully, my reluctance of doing some changes that I noted would be improvements came back to kick me in the face. I did see that it would be better to change the sign and currency constants into enums, but didn’t perform that particular change. This test revealed that some methods expecting sign and currency parameters (as Strings) were being called with the parameters switched! Oh well, next time I’ll be good and create the enums when I see they’re needed.
Now, getting back to why this is slightly painful. One could manage testing different situations in this way, but it would involve a lot of copy pasting, and you would be testing different things at once. The method above, for instance, tests that we get the correct values out of the data structure in the class, which actually has quite a few possible combinations so we could have a lot of those tests. We’re also testing that we are doing the correct calculation on those numbers.
Of course, there are other methods in the class that work on a slightly higher level, for which the tests would need to cover even more combinations. Methods actually seem to be doing more than one thing. Hmm…
Splitting function from structure
So let’s simplify. What’s the simplest thing that we’re actually doing? Adding numbers! But the lowest level of granularity in the class under test is the calculateOverAllTotalForSign (which is why that’s the method tested above!). It’s shown below. It does more then just adding numbers, though, it also subtracts a number. And this method doesn’t just know how to add and subtract, it also knows which numbers it should add, and which it should subtract. That’s what makes the test above overly complex.
1
2
3
4
5
6
|
protected BigDecimal calculateOverAllTotalForSign(String currency, String sign) {
return balancedAmounts.getValue(currency, sign)
.add(recordAmounts.getValue(currency, sign)
.add(sansAmounts.getValue(currency, sign))
.subtract(faultyAccountRecordAmounts.getValue(currency, sign)));
}
|
So let’s write a test for one of the functional parts of this method. The adding part. (For those of you laughing at the ridiculousness of testing whether you can add numbers in a program, please bear with me! It’s interesting to see where we’ll end-up.)
1
2
3
4
|
public void testAddAllValues() {
assertEquals(new BigDecimal(12), calculator.addAllValues
(BigDecimal.TEN, BigDecimal.ONE, BigDecimal.ONE));
}
|
The first thing you might notice, is that the method that this test is calling does not exist! We’re red! So we create a method that makes the test pass:
1
2
3
|
protected BigDecimal addAllValues(BigDecimal...bigDecimals) {
return new BigDecimal(12);
}
|
Yes, this is rather basic. But I’m trying to do the whole TDD thing correctly, now, even if it is on an existing untested class. If you want to learn why this can be good, read the book. But we’re green. Remember, I’m assuming I don’t know what I’m doing, and taking baby steps.
1
2
3
4
5
6
7
8
9
10
|
public void testAddAllValuesForBigDecimalConstants() {
assertEquals(new BigDecimal(12), calculator.addAllValues
(BigDecimal.TEN, BigDecimal.ONE, BigDecimal.ONE));
}
public void testAddAllValuesForOtherBigDecimals() {
assertEquals(new BigDecimal(111), calculator.addAllValues
(new BigDecimal(33), new BigDecimal(34), new BigDecimal(23),
new BigDecimal(11), new BigDecimal(6), new BigDecimal(4)));
}
|
Now we’re red again. I must admit to feeling slightly uncomfortable with this second test. I don’t see the need for it, apart from forcing us to step away from the stupidly simple implementation of the method. But then, we could just remove one of the tests once we’ve done that little refactoring step.
1
2
3
4
5
6
7
|
protected BigDecimal addAllValues(BigDecimal...bigDecimals) {
BigDecimal result = BigDecimal.ZERO;
for (BigDecimal bigDecimal : bigDecimals) {
result = result.add(bigDecimal);
}
return result;
}
|
Simple enough, and we’re green again! Let’s figure out what that does to our calculateOverAllTotalForSign method.
1
2
3
4
5
6
|
protected BigDecimal calculateOverAllTotalForSign(String currency, String sign) {
return addAllValues(balancedAmounts.getValue(currency, sign),
recordAmounts.getValue(currency, sign),
sansAmounts.getValue(currency, sign))
.subtract(faultyAccountRecordAmounts.getValue(currency, sign));
}
|
We could take this one step further, since the subtraction is still part of the calculate method, together with the selection of the values to calculate on. Might be useful, let’s see.
1
2
3
4
|
public void testDifference() {
assertEquals(new BigDecimal(9),
calculator.calculateDifference(BigDecimal.TEN, BigDecimal.ONE));
}
|
1
2
3
|
protected BigDecimal calculateDifference(BigDecimal base, BigDecimal toSubtract) {
return new BigDecimal(9);
}
|
Let’s just do the refactor step without an additional test.
1
2
3
|
protected BigDecimal calculateDifference(BigDecimal base, BigDecimal toSubtract) {
return base.subtract(toSubtract);
}
|
Which gives us:
1
2
3
4
5
6
7
|
protected BigDecimal calculateOverAllTotalForSign(String currency, String sign) {
return calculateDifference(
addAllValues(balancedAmounts.getValue(currency, sign),
recordAmounts.getValue(currency, sign),
sansAmounts.getValue(currency, sign)),
faultyAccountRecordAmounts.getValue(currency, sign));
}
|
I’m still not sure about just wrapping the BigDecimal.subtract method, but this does look a little more readable to me. And the calculations are actually tested a bit. This that I don’t trust Java to do adding and subtraction, but the test make it more explicit that this calculation is always adding a list of numbers, and subtracting one other number. That’s the other side of testing, documentation of the intention of your code.
Let’s continue on to the next existing method we can test, which would be
1
2
3
4
|
private BigDecimal calculateOverallTotal(String currency) {
return calculateOverAllTotalForSign(currency, Constants.CREDIT)
.subtract(calculateOverAllTotalForSign(currency, Constants.DEBIT)).abs();
}
|
This method is calling the one we just went through, once for CREDIT, once for DEBIT, and then takes the difference (hey!) between the two. Oh, and then returns the absolute value of that difference. Should be easy to test, right? Nope.
The problem is that the calculateOverAllTotalForSign method uses the fields of the class, which makes any method that uses it hard to test in isolation. But there’s no reason why we can’t separate the input (calls to calculateOverAllTotalForSign) from the operations, even in such a simple method as this one.
First step could be a simple refactoring of the method, using the calculateDifference method we just created:
1
2
3
4
|
private BigDecimal calculateOverallTotal(String currency) {
return calculateDifference(calculateOverAllTotalForSign(currency, Constants.CREDIT),
calculateOverAllTotalForSign(currency, Constants.DEBIT)).abs();
}
|
Ok. Not too interesting, we should go a little further, and create a simple test.
1
2
3
|
public void testCalculateAbsoluteDifferencePositive() {
assertEquals(new BigDecimal(30), calculator.calculateAbsoluteDifference(new BigDecimal(45), new BigDecimal(15)));
}
|
This introduces the calculateAbsoluteDifference method.
1
2
3
|
protected BigDecimal calculateAbsoluteDifference(BigDecimal base, BigDecimal toSubtract) {
return calculateDifference(base, toSubtract);
}
|
Perfectly useless, of course, since it just calls the normal calculateDifference method. That’s ok, though. Just add the test where the absolutes matter.
1
2
3
|
public void testCalculateAbsoluteDifferenceNegative() {
assertEquals(new BigDecimal(30), calculator.calculateAbsoluteDifference(new BigDecimal(15), new BigDecimal(45)));
}
|
And we’re red again. So we add the abs() call:
1
2
3
|
protected BigDecimal calculateAbsoluteDifference(BigDecimal base, BigDecimal toSubtract) {
return calculateDifference(base, toSubtract).abs();
}
|
Which makes the calculateOverallTotal call into:
1
2
3
4
|
protected BigDecimal calculateOverallTotal(String currency) {
return calculateAbsoluteDifference(calculateOverAllTotalForSign(currency, Constants.DEBIT),
calculateOverAllTotalForSign(currency, Constants.CREDIT));
}
|
So we separated the actual calculation from the decision on what the values are to calculate. That’s good, and if we’d made any mistakes in the calculation part (such as inverting the order, or something similar) we’d have noticed, and had a clear indication of where we’d made the mistakes.
What’s interesting is that we’ve now already tested most of the calculations in this class. There’s another one (the adding-up one in CreditDebitHolder), and the rest of the class is doing bookkeeping. So we need to test the adding-up, and we need to test the bookkeeping.
On to testing the data structures
Let’s start with the bookkeeping. The CreditDebitHolder is keeping values, storing them with a combination of sign and currency as key. It’s externally useful methods are addToValue and getValue. We also have a setValue method, but it seems to only be used as part of our testCalculateTotalForCurrencyAndSign test. We’ll leave that one for now, and first create a test for the combination of addToValue and getValue.
1
2
3
4
|
public void testAddSingleValueToEmptyHolder() {
calculator.recordAmounts.addToValue(Constants.CURRENCY_EURO, Constants.DEBIT, bd(10));
assertEquals(bd(10), calculator.recordAmounts.getValue(Constants.CURRENCY_EURO, Constants.DEBIT));
}
|
Note: I’ve extracted the new BigDecimal(<some number>) that was making our tests hard to read into the bd(<some number>) method. Just for readability.
But… Now we’re testing a combination of methods, not a single method. Is that good? I’m not 100% sure. But any other option involves poking into this inner class’ privates, which I don’t think is a great idea. Let’s test some more situations and see what happens.
1
2
3
4
5
6
|
public void testGetValueFromEmptyHolder() {
assertEquals(bd(0), calculator.recordAmounts.getValue(CURRENCY_EURO, DEBIT));
assertEquals(bd(0), calculator.recordAmounts.getValue(CURRENCY_USD, DEBIT));
assertEquals(bd(0), calculator.recordAmounts.getValue(CURRENCY_EURO, CREDIT));
assertEquals(bd(0), calculator.recordAmounts.getValue(CURRENCY_USD, CREDIT));
}
|
Note: Another small improvement in readability by making the Constants import static.
So we do indeed get back a value of `` if we don’t store anything in there. Good to know. But we’re testing side effects. Is there any way we can test this directly? Of course!
1
2
3
|
public void testReplaceNullValueWithZeroForValidValue() {
assertEquals(bd(3), calculator.recordAmounts.replaceNullValueWithZero(bd(3)));
}
|
And if we create that method:
1
2
3
|
public BigDecimal replaceNullValueWithZero(BigDecimal value) {
return value;
}
|
Then we add the test for the actual passing in of a null value:
1
2
3
|
public void testReplaceNullValueWithZeroForNullValue() {
assertEquals(bd(0), calculator.recordAmounts.replaceNullValueWithZero(null));
}
|
Which, after a short stop on red again, brings us:
1
2
3
4
5
6
7
8
9
10
11
|
public BigDecimal getValue(String currency, String sign) {
BigDecimal value = values.get(sign).get(currency);
return replaceNullValueWithZero(value);
}
public BigDecimal replaceNullValueWithZero(BigDecimal value) {
if (value == null) {
value = BigDecimal.ZERO;
}
return value;
}
|
I know that for many people taking this decomposition this far is overdoing it. But I can now test this particular safeguard separately, instead of as a side effect. I like that. Also, the getValue method just got a little more readable.
Another situation we can test is what happens when we have a value set for a currency/sign combination, and then add a value:
1
2
3
4
5
|
public void testAddSingleValueToHolderWithValueSet() {
calculator.recordAmounts.setValue(CURRENCY_EURO, DEBIT, bd(10));
calculator.recordAmounts.addToValue(CURRENCY_EURO, DEBIT, bd(10));
assertEquals(bd(20), calculator.recordAmounts.getValue(CURRENCY_EURO, DEBIT));
}
|
Almost disappointingly, this just works. But! Now that I look at those methods right above each other, I do see that we have a little code duplication here: addToValue actually sets the value directly, instead of using the setValue method. Let’s change that.
1
2
3
4
5
|
public void addToValue(String currency, String sign, BigDecimal value) {
BigDecimal oldValue = getValue(currency, sign);
BigDecimal newValue = oldValue.add(value);
setValue(currency, sign, newValue);
}
|
A small change, but satisfying. Now let’s test some other situations for currency/sign combinations.
1
2
3
4
5
6
7
8
9
10
11
12
13
|
public void testAddSingleValueToHolderWithValueForOtherSignSet() {
calculator.recordAmounts.setValue(CURRENCY_EURO, CREDIT, bd(10));
calculator.recordAmounts.addToValue(CURRENCY_EURO, DEBIT, bd(10));
assertEquals(bd(10), calculator.recordAmounts.getValue(CURRENCY_EURO, DEBIT));
assertEquals(bd(10), calculator.recordAmounts.getValue(CURRENCY_EURO, CREDIT));
}
public void testAddSingleValueToHolderWithValueForOtherCurrencySet() {
calculator.recordAmounts.setValue(CURRENCY_USD, DEBIT, bd(10));
calculator.recordAmounts.addToValue(CURRENCY_EURO, DEBIT, bd(10));
assertEquals(bd(10), calculator.recordAmounts.getValue(CURRENCY_USD, DEBIT));
assertEquals(bd(10), calculator.recordAmounts.getValue(CURRENCY_EURO, DEBIT));
}
|
These are typical test-after tests, but they at least test that we’re keeping our values nicely separate.
So, I think this gives me full code coverage for the CreditDebitHolder. But not for the rest of the ConfirmationLetterTotalsCalculator class yet, so we need to look where we still have testing to add.
We still have to test the different filters, which determine which records are included in which totals. And the methods looping over the different combinations of record types and currencies to call the calculations we’ve already tested.
Testing at a slightly higher level
Let’s work our way up, and see what happens: start with calculateOverallTotalsForAllCurrencies. All that method does is go over all currencies (defined in the CURRENCIES constant), and call the already tested calculateOverallTotal.
To test this, I would set-up recordAmounts with some different values for sign/currency combinations, and check the result. But wait! We did test calculateAbsoluteDifference, but we never really tested calculateOverallTotal or calculateOverAllTotalForSign! We did test that once values are in the recordAmounts structure, we get them out again correctly. But we should test that we’re subtracting the right values, shouldn’t we?
1
2
3
4
5
6
7
8
9
|
public void testCalculateOverAllTotalForSign() {
// numbers that should be added
calc.balancedAmounts.setValue(CURRENCY_USD, DEBIT, bd(5));
calc.recordAmounts.setValue(CURRENCY_USD, DEBIT, bd(5));
calc.sansAmounts.setValue(CURRENCY_USD, DEBIT, bd(5));
// number that should be subtracted
calc.faultyAccountRecordAmounts.setValue(CURRENCY_USD, DEBIT, bd(3));
assertEquals(bd(12), calc.calculateOverAllTotalForSign(CURRENCY_USD, DEBIT));
}
|
Luckily, we don’t have to worry about testing the calculations, we already know that those work. So all we’re interested here is that the right numbers are being added, and the right one is being subtracted.
I don’t like this test very much, though. I think that has mostly to do with the fact that I really need those comments in there to show what the intention of the test is.
Oh dear
What I’m seeing (smelling?) here is that when I use my code from another place, it becomes clear that it is not very understandable by itself. That probably means that I need to take another look at the structure of the class, and see why that is. The method we’re testing is simple. It just adds a couple of numbers, and subtracts another. That’s Ok. The problem is that from the code, it is not clear why it’s those particular numbers we need to add, and why that single one to subtract.
In fact, in de pursuit of de-duplication, we’re always adding three numbers, while in the ‘balanced’ case (see the calculateRetrievedAmounts entry-point method to the class) only the balancedAmounts is used (so all other values are empty, and thus 0!), while in the unbalanced case, the balancedAmounts is guaranteed to be empty.
So we’re trying to handle two very different situations in one class. The Single Responsibility Principle Police is not going to like that! Damn, does that mean we need to refactor what we have into separate classes? Just because I don’t like the readability of a test? Yes, and no.
Yes, I need to refactor. No, it’s not the readability of the test. That is just how we found out that the class under test is not as readable as we thought it was.
Lessee… The decision on how to calculate is based on the client.isBalanced() boolean. The client object is used in a few other places, mostly to set a default into the record when the sign or currency wasn’t set in there. Now that I look at that, it would probably be better if those calls moved into the record class(es)…
As for this class, what if we replaced the generic calculator that we have with an two more specific ones, created by a little factory, based on the balanced boolean…
No, let’s see where this takes us, step by step. First, I’ll move out those methods setting defaults to the Record class(es). This involves some changes, which I won’t go into detail in here, but involves making GenericRecord an abstract class, moving these methods into it, and moving the test for them into a new test class for GenericRecord. I’ve refrained from calling these automatically when getting the sign or currency from a record, since I don’t know whether that will break anything in the rest of the code-base. As an indication that this is moving in the right direction all code involved, including tests, is getting slightly cleaner, simpler and more specific.
conclusion
To avoid making this into another/even-more unreadably-long post, I’m going to cut it off here and continue with the next steps in a new post. So far, we’ve found that we could do some small improvements to our code-base when we start seriously looking at testability. More importantly, though, we’ve seen that when trying to test, we automatically start noticing some things that we previously didn’t. This means that we’re learning more about the code, and shows that the function of the tests to document the intention of our code is working and valuable.
Stay tuned for the exciting finale! :-)