One of the things I find myself doing at work is looking at other peoples code. This is not unusual, of course, as every programmer does that all the time. Even if the ‘other people’ is him, last week. As all you programmers know, rather often ‘other people’s code’ is not very pretty. Partly, this can be explained because every programmer knows, no one is quite as good at programming as himself… But very often, way too often, the code really is not all that good.
This can be caused by many things. Sometimes the programmers are not very experienced. Sometimes the pressure to release new features is such that programmers feel pressured into cutting quality. Sometimes the programmers found the code in that state, and simply didn’t know where to start to improve things. Some programmers may not even have read Clean Code, Refactoring, or the Pragmatic Programmer! And maybe no one ever told them they should.
Recently I was asked to look at a Java codebase, to see if it would be possible for our company to take that into a support contract. Or what would be needed to get it to that state. This codebase had a number of problems, with lack of tests, lots of code duplication and a very uneven distribution in complexity (lots of ‘struct’ classes and the logic that should be in them spread out, and duplicated, over the rest). There was plenty wrong, and sonar quickly showed most of them.
When discussing the issues with this particular code base, I noticed that the developers already knew quite a few of the things that were wrong. They did not have a clear idea of how to go from there towards a good state, though. To illustrate how one might approach this, I spent a day making an example out of one of the high complexity classes (cyclomatic complexity of 98).
Larger examples of refactoring are fairly rare out there, so I figured I’d share this. Of course, package and class names (and some constants/variables) have been altered to protect the innocent.
I’d like to emphasize that none of this is very special. I’m not a wizard at doing this, by any standard. I don’t even code full time nowadays. That’s irrelevant: The point here is precisely that by taking a series of very simple and straightforward steps, you can improve your code tremendously. Anyone can do this! Everyone should…
I don’t usually shield off part of my posts under a ‘read-more’ link, but this post had become HUGE, and I don’t want to harm any unsuspecting RSS readers out there. Please, do read the whole thing. And: Let me (and my reader and colleagues) know how this can be done better!
Let’s start with the ‘before’ state. We have a class of about 680 lines, which is meant to generate a letter as a PDF, which is returned as an ouput stream. To do this, the class first starts to do some calculations, though it’s not completely clear what those calculation are when we first start. Sonar reports the class as having a Cyclomatic Complexity of 98 (there are a few classes more complex in the project, but not many).
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
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
503
504
505
506
507
508
509
510
511
512
513
514
515
516
517
518
519
520
521
522
523
524
525
526
527
528
529
530
531
532
533
534
535
536
537
538
539
540
541
542
543
544
545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591
592
593
594
595
596
597
598
599
600
601
602
603
604
605
606
607
608
609
610
611
612
613
614
615
616
617
618
619
620
621
622
623
624
625
626
627
628
629
630
631
632
633
634
635
636
637
638
639
640
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
|
package com.example.confirmationletter;
import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.springframework.webflow.execution.RequestContext;
import com.example.dao.CurrencyDao;
import com.example.domain.AmountAndRecordsPerBank;
import com.example.domain.BatchTotal;
import com.example.domain.Client;
import com.example.domain.ConfirmationLetter;
import com.example.domain.Currency;
import com.example.domain.HashBatchRecordsBalance;
import com.example.domain.Record;
import com.example.record.command.FileUploadCommand;
import com.example.record.domain.TempRecord;
import com.example.record.parser.FileExtension;
import com.example.record.service.impl.Constants;
public class ConfirmationLetterGenerator {
@SuppressWarnings("unused")
private static Log logger = LogFactory.getLog(ConfirmationLetterGenerator.class);
private static final String COLLECTIVE = "collectief";
private String crediting;
private String debiting;
private String debit;
private String credit;
private String type;
private LetterSelector letterSelector;
private CurrencyDao currencyDao;
public CurrencyDao getCurrencyDao() {
return currencyDao;
}
public void setCurrencyDao(CurrencyDao currencyDao) {
this.currencyDao = currencyDao;
}
public OurOwnByteArrayOutputStream letter(RequestContext context,
FileUploadCommand fileUploadCommand, Client client,
HashBatchRecordsBalance hashBatchRecordsBalance, String branchName,
List<AmountAndRecordsPerBank> bankMap,
List<com.example.record.domain.FaultRecord> faultyRecords,
FileExtension extension, List<Record> records,
List<TempRecord> faultyAccountNumberRecordList,
List<TempRecord> sansDuplicateFaultRecordsList
// ,
// List<BigDecimal> retrievedAmountFL,
// List<BigDecimal> retrievedAmountEUR,
// List<BigDecimal> retrievedAmountUSD
) {
ConfirmationLetter letter = new ConfirmationLetter();
letter.setCurrency(records.get(0).getCurrency());
letter.setExtension(extension);
letter.setHashTotalCredit(hashBatchRecordsBalance.getHashTotalCredit()
.toString());
letter.setHashTotalDebit(hashBatchRecordsBalance.getHashTotalDebit()
.toString());
letter.setBatchTotalDebit(debitBatchTotal(
hashBatchRecordsBalance.getBatchTotals(), client).toString());
letter.setBatchTotalCredit(creditBatchTotal(
hashBatchRecordsBalance.getBatchTotals(), client).toString());
letter.setTotalProcessedRecords(hashBatchRecordsBalance
.getRecordsTotal().toString());
if (fileUploadCommand.getFee().equalsIgnoreCase(Constants.YES)) {
letter.setTransactionCost(hashBatchRecordsBalance.getTotalFee()
.toString());
} else
letter.setTransactionCost("");
letter.setTransferType(hashBatchRecordsBalance.getCollectionType());
// // logger.debug("letter method, bankMap: "+bankMap.size());
letter.setBanks(bankMap);
// uncommented this line
letter.setCreditingErrors(faultyRecords);
letter.setClient(client);
letter.setBranchName(branchName);
Map<String, BigDecimal> retrievedAmounts = new HashMap<String, BigDecimal>();
retrievedAmounts = calculateRetrieveAmounts(records, faultyRecords,
client, extension, faultyAccountNumberRecordList,
sansDuplicateFaultRecordsList);
letter.setRetrievedAmountEur(retrievedAmounts
.get(Constants.CURRENCY_EURO));
letter.setRetrievedAmountFL(retrievedAmounts
.get(Constants.CURRENCY_FL));
letter.setRetrievedAmountUsd(retrievedAmounts
.get(Constants.CURRENCY_FL));
// System.out.println("TRACING AMOUNT ["+letter.getRetrievedAmountFL()+"]");
// letter.setRetrievedAmountFLDBF(retrievedAmounts.get("FLDBF"));
// letter.setRetrievedAmountUSDDBF(retrievedAmounts.get("USDDBF"));
// letter.setRetrievedAmountEURDBF(retrievedAmounts.get("EURDBF"));
letter.setTotalRetrievedRecords(fileUploadCommand.getTotalRecords());
OurOwnByteArrayOutputStream arrayOutputStream = letterSelector
.generateLetter(client.getCreditDebit(), letter);
context.getConversationScope().asMap().put("dsbByteArrayOutputStream",
arrayOutputStream);
return arrayOutputStream;
}
// Calculate sum amount from faultyAccountnumber list
private Map<String, BigDecimal> calculateAmountsFaultyAccountNumber(
List<TempRecord> faultyAccountNumberRecordList, Client client) {
Map<String, BigDecimal> retrievedAmountsFaultyAccountNumber = new HashMap<String, BigDecimal>();
BigDecimal faultyAccRecordAmountCreditFL = new BigDecimal(0);
BigDecimal faultyAccRecordAmountCreditUSD = new BigDecimal(0);
BigDecimal faultyAccRecordAmountCreditEUR = new BigDecimal(0);
BigDecimal faultyAccRecordAmountDebitFL = new BigDecimal(0);
BigDecimal faultyAccRecordAmountDebitUSD = new BigDecimal(0);
BigDecimal faultyAccRecordAmountDebitEUR = new BigDecimal(0);
for (TempRecord faultyAccountNumberRecord : faultyAccountNumberRecordList) {
// // logger.debug("faultyAccountNumberRecord: "+
// faultyAccountNumberRecord);
// FL
if (StringUtils.isBlank(faultyAccountNumberRecord.getSign())) {
faultyAccountNumberRecord.setSign(client.getCreditDebit());
}
if (faultyAccountNumberRecord.getCurrencycode() == null) {
String currencyId = currencyDao.retrieveCurrencyDefault(client
.getProfile());
Currency currency = currencyDao
.retrieveCurrencyOnId(new Integer(currencyId));
faultyAccountNumberRecord.setCurrencycode(currency.getCode()
.toString());
}
if (faultyAccountNumberRecord.getCurrencycode().equals(
Constants.FL_CURRENCY_CODE)
|| faultyAccountNumberRecord.getCurrencycode().equals(
Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK)) {
if (faultyAccountNumberRecord.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
faultyAccRecordAmountDebitFL = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountDebitFL);
} else {
faultyAccRecordAmountCreditFL = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountCreditFL);
}
}
if (faultyAccountNumberRecord.getCurrencycode().equals(
Constants.USD_CURRENCY_CODE)) {
if (faultyAccountNumberRecord.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
faultyAccRecordAmountDebitUSD = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountDebitUSD);
} else {
faultyAccRecordAmountCreditUSD = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountCreditUSD);
}
}
if (faultyAccountNumberRecord.getCurrencycode().equals(
Constants.EUR_CURRENCY_CODE)) {
if (faultyAccountNumberRecord.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
faultyAccRecordAmountDebitEUR = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountDebitEUR);
} else {
faultyAccRecordAmountCreditEUR = new BigDecimal(
faultyAccountNumberRecord.getAmount())
.add(faultyAccRecordAmountCreditEUR);
}
}
retrievedAmountsFaultyAccountNumber.put("FaultyAccDebitFL",
faultyAccRecordAmountDebitFL);
retrievedAmountsFaultyAccountNumber.put("FaultyAccDebitUSD",
faultyAccRecordAmountDebitUSD);
retrievedAmountsFaultyAccountNumber.put("FaultyAccDebitEUR",
faultyAccRecordAmountDebitEUR);
retrievedAmountsFaultyAccountNumber.put("FaultyAccCreditFL",
faultyAccRecordAmountCreditFL);
retrievedAmountsFaultyAccountNumber.put("FaultyAccCreditUSD",
faultyAccRecordAmountCreditUSD);
retrievedAmountsFaultyAccountNumber.put("FaultyAccCreditEUR",
faultyAccRecordAmountCreditEUR);
}
return retrievedAmountsFaultyAccountNumber;
}
private Map<String, BigDecimal> calculateRetrieveAmounts(
List<Record> records,
List<com.example.record.domain.FaultRecord> faultyRecords,
Client client, FileExtension extension,
List<TempRecord> faultyAccountNumberRecordList,
List<TempRecord> sansDuplicateFaultRecordsList) {
Map<String, BigDecimal> retrievedAmounts = new HashMap<String, BigDecimal>();
BigDecimal recordAmountFL = new BigDecimal(0);
BigDecimal recordAmountUSD = new BigDecimal(0);
BigDecimal recordAmountEUR = new BigDecimal(0);
BigDecimal recordAmountDebitFL = new BigDecimal(0);
BigDecimal recordAmountDebitEUR = new BigDecimal(0);
BigDecimal recordAmountDebitUSD = new BigDecimal(0);
BigDecimal recordAmountCreditFL = new BigDecimal(0);
BigDecimal recordAmountCreditEUR = new BigDecimal(0);
BigDecimal recordAmountCreditUSD = new BigDecimal(0);
BigDecimal amountSansDebitFL = new BigDecimal(0);
BigDecimal amountSansDebitUSD = new BigDecimal(0);
BigDecimal amountSansDebitEUR = new BigDecimal(0);
BigDecimal amountSansCreditFL = new BigDecimal(0);
BigDecimal amountSansCreditUSD = new BigDecimal(0);
BigDecimal amountSansCreditEUR = new BigDecimal(0);
BigDecimal totalDebitFL = new BigDecimal(0);
BigDecimal totalDebitUSD = new BigDecimal(0);
BigDecimal totalDebitEUR = new BigDecimal(0);
BigDecimal totalCreditFL = new BigDecimal(0);
BigDecimal totalCreditUSD = new BigDecimal(0);
BigDecimal totalCreditEUR = new BigDecimal(0);
if (client.getCounterTransfer().equalsIgnoreCase(Constants.TRUE)) {
for (Record record : records) {
if (record.getFeeRecord() != 1) {
if ((record.getCurrency().getCode().equals(
Constants.FL_CURRENCY_CODE) || record
.getCurrency().getCode().equals(
Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK))
&& record.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
recordAmountFL = record.getAmount().add(
recordAmountFL);
// system.out.println("recordAmountFL: ["+ recordAmountFL + "]");
}
if (record.getCurrency().getCode().equals(
Constants.EUR_CURRENCY_CODE)
&& record.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
recordAmountEUR = record.getAmount().add(
recordAmountEUR);
// system.out.println("recordAmountEUR: ["+ recordAmountEUR + "]");
}
if (record.getCurrency().getCode().equals(
Constants.USD_CURRENCY_CODE)
&& record.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
recordAmountUSD = record.getAmount().add(
recordAmountUSD);
// system.out.println("recordAmountUSD: ["+ recordAmountUSD + "]");
}
}
retrievedAmounts.put(Constants.CURRENCY_EURO, recordAmountEUR);
retrievedAmounts.put(Constants.CURRENCY_FL, recordAmountUSD);
retrievedAmounts.put(Constants.CURRENCY_FL, recordAmountFL);
}
}
// Not Balanced
else {
for (Record record : records) {
logger.debug("COUNTERTRANSFER ["+record.getIsCounterTransferRecord()+"] FEERECORD ["+record.getFeeRecord()+"]");
if (record.getIsCounterTransferRecord().compareTo(new Integer(0))==0
&& record.getFeeRecord().compareTo(new Integer(0))==0) {
if ((record.getCurrency().getCode().equals(
Constants.FL_CURRENCY_CODE) || record
.getCurrency().getCode().equals(
Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK))) {
// System.out.println("record to string: ["+record.toString()+"]");
if (record.getSign().equalsIgnoreCase(Constants.DEBIT)) {
// System.out.println("record.getamount DEBIT = ["+ record.getAmount() + "]");
// system.out.println("recordAmountDebitFL 1 = "+ recordAmountDebitFL);
recordAmountDebitFL = record.getAmount().add(
recordAmountDebitFL);
// System.out.println("recordAmountDebitFL: ["+recordAmountDebitFL+"]");
}
if (record.getSign().equalsIgnoreCase(Constants.CREDIT)) {
// System.out.println("record.getamount CREDIT = ["+record.getAmount()+"]");
// system.out.println("recordAmountCreditFL 1 = ["+recordAmountCreditFL+"]");
recordAmountCreditFL = record.getAmount().add(
recordAmountCreditFL);
// System.out.println("recordAmountCreditFL: ["+recordAmountCreditFL+"]");
}
if (record.getCurrency().getCode().equals(
Constants.EUR_CURRENCY_CODE)) {
if (record.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
recordAmountDebitEUR = record.getAmount().add(
recordAmountDebitEUR);
// system.out.println("recordAmountDebitEUR: ["+recordAmountDebitEUR+"]");
}
if (record.getSign().equalsIgnoreCase(
Constants.CREDIT)) {
recordAmountCreditEUR = record.getAmount().add(
recordAmountCreditEUR);
// system.out.println("recordAmountCreditEUR: ["+recordAmountCreditEUR+"]");
}
}
}
}
if (record.getCurrency().getCode().equals(
Constants.USD_CURRENCY_CODE)) {
if (record.getSign().equalsIgnoreCase(Constants.DEBIT)) {
recordAmountDebitUSD = record.getAmount().add(
recordAmountDebitUSD);
// system.out.println("recordAmountDebitUSD: ["+recordAmountDebitUSD+"]");
}
if (record.getSign().equalsIgnoreCase(Constants.CREDIT)) {
recordAmountCreditUSD = record.getAmount().add(
recordAmountCreditUSD);
// system.out.println("recordAmountCreditUSD: ["+recordAmountCreditUSD+"]");
}
}
}
// Sansduplicate
for (TempRecord sansDupRec : sansDuplicateFaultRecordsList) {
// logger.debug("sansDupRec: "+sansDupRec);
String currencyCode = sansDupRec.getCurrencycode();
if (sansDupRec.getSign() == null) {
String sign = client.getCreditDebit();
sansDupRec.setSign(sign);
}
if (currencyCode == null) {
String currencyId = currencyDao
.retrieveCurrencyDefault(client.getProfile());
Currency currency = currencyDao
.retrieveCurrencyOnId(new Integer(currencyId));
sansDupRec.setCurrencycode(currency.getCode().toString());
} else {
if (currencyCode.equals(Constants.FL_CURRENCY_CODE)
|| currencyCode
.equals(Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK)) {
if (sansDupRec.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
amountSansDebitFL = new BigDecimal(sansDupRec
.getAmount()).add(amountSansDebitFL);
} else {
amountSansCreditFL = new BigDecimal(sansDupRec
.getAmount()).add(amountSansCreditFL);
}
}
if (currencyCode.equals(Constants.USD_CURRENCY_CODE)) {
if (sansDupRec.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
amountSansDebitUSD = new BigDecimal(sansDupRec
.getAmount()).add(amountSansDebitUSD);
} else {
amountSansCreditUSD = new BigDecimal(sansDupRec
.getAmount()).add(amountSansCreditUSD);
}
}
if (currencyCode.equals(Constants.EUR_CURRENCY_CODE)) {
if (sansDupRec.getSign().equalsIgnoreCase(
Constants.DEBIT)) {
amountSansDebitEUR = new BigDecimal(sansDupRec
.getAmount()).add(amountSansDebitEUR);
} else {
amountSansCreditEUR = new BigDecimal(sansDupRec
.getAmount()).add(amountSansCreditEUR);
}
}
}
}
Map<String, BigDecimal> retrievedAccountNumberAmounts = calculateAmountsFaultyAccountNumber(
faultyAccountNumberRecordList, client);
// logger.info("Before total debit FL");
// logger.info("amountSansDebitFL "+amountSansDebitFL);
if (retrievedAccountNumberAmounts.get("FaultyAccDebitFL") != null
&& amountSansDebitFL != null) {
// logger.info("retrievedAccountNumberAmounts.get(FaultyAccDebitFL) "+retrievedAccountNumberAmounts.get("FaultyAccDebitFL"));
totalDebitFL = recordAmountDebitFL.add(amountSansDebitFL)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccDebitFL"));
} else if (amountSansDebitFL != null) {
totalDebitFL = recordAmountDebitFL.add(amountSansDebitFL);
} else {
totalDebitFL = recordAmountDebitFL;
}
// logger.info("totalDebitFL "+totalDebitFL);
if (retrievedAccountNumberAmounts.get("FaultyAccCreditFL") != null
&& amountSansCreditFL != null) {
// logger.debug("retrievedAccountNumberAmounts.get(FaultyAccCreditFL):"+retrievedAccountNumberAmounts.get("FaultyAccCreditFL"));
totalCreditFL = recordAmountCreditFL.add(amountSansCreditFL)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccCreditFL"));
} else if (amountSansCreditFL != null) {
totalCreditFL = recordAmountCreditFL.add(amountSansCreditFL);
} else {
totalCreditFL = recordAmountCreditFL;
}
// logger.info("totalCreditFL: "+totalCreditFL);
if (retrievedAccountNumberAmounts.get("FaultyAccDebitUSD") != null
&& amountSansDebitUSD != null) {
// logger.info("retrievedAccountNumberAmounts.get(FaultyAccDebitUSD) "+retrievedAccountNumberAmounts.get("FaultyAccDebitUSD"));
totalDebitUSD = recordAmountDebitUSD.add(amountSansDebitUSD)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccDebitUSD"));
} else if (amountSansDebitUSD != null) {
totalDebitUSD = recordAmountDebitUSD.add(amountSansDebitUSD);
} else {
totalDebitUSD = recordAmountDebitUSD;
}
// logger.info("totalDebitUSD "+totalDebitUSD);
if (retrievedAccountNumberAmounts.get("FaultyAccCreditUSD") != null
&& amountSansCreditUSD != null) {
// logger.debug("retrievedAccountNumberAmounts.get(FaultyAccCreditUSD):"+retrievedAccountNumberAmounts.get("FaultyAccCreditUSD"));
totalCreditUSD = recordAmountCreditUSD.add(amountSansCreditUSD)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccCreditUSD"));
} else if (amountSansCreditUSD != null) {
totalCreditUSD = recordAmountCreditUSD.add(amountSansCreditUSD);
} else {
totalCreditUSD = recordAmountCreditUSD;
}
// logger.info("totalCreditUSD: "+totalCreditUSD);
if (retrievedAccountNumberAmounts.get("FaultyAccDebitEUR") != null
&& amountSansDebitEUR != null) {
// logger.info("retrievedAccountNumberAmounts.get(FaultyAccDebitEUR) "+retrievedAccountNumberAmounts.get("FaultyAccDebitEUR"));
totalDebitEUR = recordAmountDebitEUR.add(amountSansDebitEUR)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccDebitEUR"));
} else if (amountSansDebitEUR != null) {
totalDebitEUR = recordAmountDebitEUR.add(amountSansDebitEUR);
} else {
totalDebitEUR = recordAmountDebitEUR;
}
// logger.info("totalDebitEUR "+totalDebitEUR);
if (retrievedAccountNumberAmounts.get("FaultyAccCreditEUR") != null
&& amountSansCreditEUR != null) {
// logger.debug("retrievedAccountNumberAmounts.get(FaultyAccCreditEUR):"+retrievedAccountNumberAmounts.get("FaultyAccCreditEUR"));
totalCreditEUR = recordAmountCreditEUR.add(amountSansCreditEUR)
.subtract(
retrievedAccountNumberAmounts
.get("FaultyAccCreditEUR"));
} else if (amountSansCreditEUR != null) {
totalCreditEUR = recordAmountCreditEUR.add(amountSansCreditEUR);
} else {
totalCreditEUR = recordAmountCreditEUR;
}
// logger.info("totalCreditEUR: "+totalCreditEUR);
recordAmountFL = totalDebitFL.subtract(totalCreditFL).abs();
recordAmountUSD = totalDebitUSD.subtract(totalCreditUSD).abs();
recordAmountEUR = totalDebitEUR.subtract(totalCreditEUR).abs();
retrievedAmounts.put(Constants.CURRENCY_EURO, recordAmountEUR);
retrievedAmounts.put(Constants.CURRENCY_FL, recordAmountUSD);
retrievedAmounts.put(Constants.CURRENCY_FL, recordAmountFL);
}
return retrievedAmounts;
}
private BigDecimal creditBatchTotal(Map<Integer, BatchTotal> batchTotals,
Client client) {
Double sum = new Double(0);
Iterator<BatchTotal> itr = batchTotals.values().iterator();
while (itr.hasNext()) {
BatchTotal total = itr.next();
sum = sum + total.getCreditValue().doubleValue();
}
Double d = sum / new Double(client.getAmountDivider());
return new BigDecimal(d);
}
private BigDecimal debitBatchTotal(Map<Integer, BatchTotal> batchTotals,
Client client) {
Double sum = new Double(0);
Iterator<BatchTotal> itr = batchTotals.values().iterator();
while (itr.hasNext()) {
BatchTotal total = itr.next();
sum = sum + total.getCreditCounterValueForDebit().doubleValue();
}
Double d = sum / new Double(client.getAmountDivider());
return new BigDecimal(d);
}
private List<AmountAndRecordsPerBank> amountAndRecords(
List<Record> records, String transactionType) {
List<AmountAndRecordsPerBank> list = new ArrayList<AmountAndRecordsPerBank>();
String typeOfTransaction = transactionType.equalsIgnoreCase(crediting) ? crediting
: debiting;
type = typeOfTransaction.equalsIgnoreCase(crediting) ? credit : debit;
if (transactionType.equalsIgnoreCase(typeOfTransaction)) {
for (Record record : records) {
getAmountAndRecords(record, list, transactionType);
}
}
return list;
}
private List<AmountAndRecordsPerBank> getAmountAndRecords(Record record,
List<AmountAndRecordsPerBank> list, String transactionType) {
Map<String, String> map = new HashMap<String, String>();
if (record.getFeeRecord().compareTo(0) == 0
&& !map.containsKey(record.getBeneficiaryName())) {
if (transactionType.equalsIgnoreCase(Constants.CREDITING)) {
if (record.getBeneficiaryName() != null
&& !record.getBeneficiaryName().equalsIgnoreCase(
Constants.RBTT_BANK_ALTERNATE)) {
Boolean newList = true;
if (list.size() == 0
&& record.getSign().equalsIgnoreCase(type)) {
// logger.info("bank gegevens: "+record.getSign()+" : "+record.getBank().getName()+" : "+record.getBeneficiaryName());
AmountAndRecordsPerBank aARPB = new AmountAndRecordsPerBank();
aARPB.setBankName(record.getBank().getName());
aARPB.setTotalRecord(1);
aARPB.setAmount(record.getAmount());
aARPB.setCurrencyType(record.getCurrency()
.getCurrencyType());
aARPB.setAccountNumber(record
.getBeneficiaryAccountNumber());
list.add(aARPB);
newList = false;
}
if (newList && record.getSign().equalsIgnoreCase(type)) {
// logger.info("bank gegevens: "+record.getSign()+" : "+record.getBank().getName()+" : "+record.getBeneficiaryName());
Boolean newRecord = true;
for (AmountAndRecordsPerBank object : list) {
if (object.getBankName().equalsIgnoreCase(
record.getBank().getName())
&& object.getCurrencyType()
.equalsIgnoreCase(
record.getCurrency()
.getCurrencyType())) {
object.setAmount(object.getAmount().add(
record.getAmount()));
object
.setTotalRecord(object.getTotalRecord() + 1);
newRecord = false;
}
}
if (newRecord) {
AmountAndRecordsPerBank aARPB = new AmountAndRecordsPerBank();
aARPB.setBankName(record.getBank().getName());
aARPB.setTotalRecord(1);
aARPB.setAmount(record.getAmount());
aARPB.setCurrencyType(record.getCurrency()
.getCurrencyType());
aARPB.setAccountNumber(record
.getBeneficiaryAccountNumber());
list.add(aARPB);
}
}
}
}
// del begin
if (transactionType.equalsIgnoreCase(Constants.DEBITING)) {
if (record.getBeneficiaryName() == null) {
Boolean newList = true;
if (list.size() == 0
&& record.getSign().equalsIgnoreCase(type)) {
// logger.info("bank gegevens: "+record.getSign()+" : "+record.getBank().getName()+" : "+record.getBeneficiaryName());
AmountAndRecordsPerBank aARPB = new AmountAndRecordsPerBank();
aARPB.setBankName(record.getBank().getName());
aARPB.setTotalRecord(1);
aARPB.setAmount(record.getAmount());
aARPB.setCurrencyType(record.getCurrency()
.getCurrencyType());
aARPB.setAccountNumber(record
.getBeneficiaryAccountNumber());
list.add(aARPB);
newList = false;
}
if (newList && record.getSign().equalsIgnoreCase(type)) {
// logger.info("bank gegevens: "+record.getSign()+" : "+record.getBank().getName()+" : "+record.getBeneficiaryName());
Boolean newRecord = true;
for (AmountAndRecordsPerBank object : list) {
if (object.getBankName().equalsIgnoreCase(
record.getBank().getName())
&& object.getCurrencyType()
.equalsIgnoreCase(
record.getCurrency()
.getCurrencyType())) {
object.setAmount(object.getAmount().add(
record.getAmount()));
object
.setTotalRecord(object.getTotalRecord() + 1);
newRecord = false;
}
}
if (newRecord) {
AmountAndRecordsPerBank aARPB = new AmountAndRecordsPerBank();
aARPB.setBankName(record.getBank().getName());
aARPB.setTotalRecord(1);
aARPB.setAmount(record.getAmount());
aARPB.setCurrencyType(record.getCurrency()
.getCurrencyType());
aARPB.setAccountNumber(record
.getBeneficiaryAccountNumber());
list.add(aARPB);
}
}
}
}
// del end
}
return list;
}
/*
*
* Getters and setters
*/
public void setCrediting(String crediting) {
this.crediting = crediting;
}
public void setDebiting(String debiting) {
this.debiting = debiting;
}
public void setDebit(String debit) {
this.debit = debit;
}
public void setCredit(String credit) {
this.credit = credit;
}
public void setLetterSelector(LetterSelector letterSelector) {
this.letterSelector = letterSelector;
}
}
|
While doing the refactoring, I wrote down the steps I took. I’ll just list those here, and where necessary explain why I did them, and how.
- Create empty unit test for class
This one is obvious: there was no unit level testing for this class. There were some integration tests, but those were dependent on an environment that I didn’t have. I was flying blind.
- Remove @suppress voor unused voor log
I’d like to see what’s going on. Either it’s not used, and we can remove it, or it is, and the suppression is unnecessary. (It was unnecessary.)
- Remove unused constant ‘COLLECTIVE’
This constant really was not being used. Maybe the @suppress was meant for this one? I don’t care, I’ll just delete it.
- Remove commented args for ’letter’
Commented out already, so I’m sure not going to break anything by removing these! And it makes things slightly more readable… This is what we have SVN for (or CVS, or Git, or…)
- Split ’letter’ into ‘createConfirmationLetter’, ‘generateConfirmationLetterAsPDF’.
The ’letter’ method is the main entrypoint for the class. But it does a couple of things. It creates a ‘ConfirmationLetter’ object, filling it with the load of parameters passed in, does a bunch of calculations, and then converts the ‘ConfirmationLetter’ object to a PDF, and returns that as a binary stream. A bit much, and the first real change is splitting it into two parts: first create the ‘ConfirmationLetter’, then convert such an object into a PDF binary.
The way we do this is simply do an ‘Extract Method’ refactoring for most of the method to get the ‘create’, and even though the PDF conversion is only one statement (the letterSelector.generateLetter call) I still gave it its own method, just to make things read naturally.
- Removed DSBByteArrayOutputStream, just use byte[] (also in PdfOutputService)
I was a bit puzzled by the passing around of this OutputStream. When I checked around in the rest of the codebase, it turned out that all the places this thing was used, all that was done with it was a toByteArray call. So I decided to get rid of the (empty) local subclass of ByteArrayOutputStream, and just pass the byte[] itself around. This involved changing the letterSelector class, of course, but we won’t be showing that here…
The ’letter’ method ends-up looking like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
|
public OurOwnByteArrayOutputStream letter(RequestContext context,
FileUploadCommand fileUploadCommand, Client client,
HashBatchRecordsBalance hashBatchRecordsBalance, String branchName,
List<AmountAndRecordsPerBank> bankMap,
List<com.example.record.domain.FaultRecord> faultyRecords,
FileExtension extension, List<Record> records,
List<TempRecord> faultyAccountNumberRecordList,
List<TempRecord> sansDuplicateFaultRecordsList
) {
ConfirmationLetter letter = createConfirmationLetter(fileUploadCommand, client, hashBatchRecordsBalance,
branchName, bankMap, faultyRecords, extension, records, faultyAccountNumberRecordList,
sansDuplicateFaultRecordsList);
OurOwnByteArrayOutputStream arrayOutputStream = generateConfirmationLetterAsPDF(client, letter);
context.getConversationScope().asMap().put("dsbByteArrayOutputStream", arrayOutputStream);
return arrayOutputStream;
}
|
Now, the more observent among the readers (and really, if you’ve gotten this far, my apologies, this is going to go on for a while longer) may have noticed something gone terribly wrong! I just created two new methods, and did not create any new tests. This is inexcusable! Bad Form. Almost naughty. I really should have started with a good integration test. But I was lazy. Bad. But. Since I didn’t really understand the code yet I figured I’d hold out on testing until I split things up far enough that I would understand what I was testing. Comments welcome:-)
So, on to further deconstruction.
- Moved calculations down in createConfirmationLetter method
Yes, that’s it. I just re-arranged the deck-chairs so that the simple filling of information is on top of the method, and the calculating stuff goes down. Simple, but helps readability, and prepares nicely for later changes. Also did some basic cleaning: get rid of commented lines of code (we have Subversion for a reason), and made sure that ’else’ in there had proper curly braces.
- Moved constant comparison to determine value of ‘fee’ into FileUploadCommand, created unit test for that method in FileUploadCommandTest
So we changed:
1
2
3
4
5
|
if (fileUploadCommand.getFee().equalsIgnoreCase(Constants.YES)) {
letter.setTransactionCost(hashBatchRecordsBalance.getTotalFee().toString());
} else {
letter.setTransactionCost("");
}
|
to:
1
2
3
4
5
|
String transactionCost = "";
if (fileUploadCommand.hasFee()) {
transactionCost = hashBatchRecordsBalance.getTotalFee().toString();
}
letter.setTransationCost(transactionCost);
|
Because it doesn’t make sense for our class to know about the internal meaning of a String value within the FileUploadCommand class. We don’t want to know why you think this file upload has a fee attached to it. Only if that is the case.
- Moved logic to determine transation cost into ‘getTransactionCost’
Then we moved the code above into its own method, making the setTransactionCost call look like
1
|
letter.setTransactionCost(getTransactionCost(fileUploadCommand, hashBatchRecordsBalance));
|
Much better, already!
- Pass in client.getAmountDivider into credit/debitBatchTotal
Because they don’t need the whole client object, just this little integer.
- Converted sum calculation in credit/debitBatchTotal into BigDecimal calc (always for finance calcs!)
This is just one of those things that you start doing after you’ve run into enough rounding issues. Really. Trust me. Also, it simplifies the code again, getting rid of some of the conversions:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
|
private BigDecimal creditBatchTotal(Map<Integer, BatchTotal> batchTotals,
Integer amountDivider) {
BigDecimal sum = BigDecimal.ZERO;
Iterator<BatchTotal> itr = batchTotals.values().iterator();
while (itr.hasNext()) {
BatchTotal total = itr.next();
sum = sum.add(total.getCreditValue());
}
return sum.divide(new BigDecimal(amountDivider));}
private BigDecimal debitBatchTotal(Map<Integer, BatchTotal> batchTotals,
Integer amountDivider) {
BigDecimal sum = BigDecimal.ZERO;
Iterator<BatchTotal> itr = batchTotals.values().iterator();
while (itr.hasNext()) {
BatchTotal total = itr.next();
sum = sum.add(total.getCreditCounterValueForDebit());
}
return sum.divide(new BigDecimal(amountDivider));
}
|
Not at all happy with this result, of course, since the methods are almost exactly the same! We should do something about that.
But wait a minute! I think that I can pretty much understand what these methods are doing! Let’s see if I can write a test. Oh wait, I can’t! The methods are private. Let’s make give ’m default access, so the test can access them.
Let’s write a first test:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
|
public class ConfirmationLetterGeneratorTest extends TestCase {
public void testCreditBatchTotal_divider_one_all_credit() {
BatchTotal one = new BatchTotal();
one.setCreditValue(BigDecimal.ONE);
BatchTotal ten = new BatchTotal();
ten.setCreditValue(BigDecimal.TEN);
HashMap<Integer, BatchTotal>batchTotals = new HashMap<Integer, BatchTotal>();
batchTotals.put(99, one);
batchTotals.put(98, ten);
ConfirmationLetterGenerator letterGenerator = new ConfirmationLetterGenerator();
BigDecimal result = letterGenerator.creditBatchTotal(batchTotals, 1);
assertEquals(new BigDecimal(11), result);
}
}
|
And indeed, after running that we can confirm that this method can add two numbers! So what happens if we have an input where not all entries in your list are CREDIT?
1
2
3
4
5
6
7
8
|
public void testCreditBatchTotal_divider_one_mixed_credit_debit() {
HashMap<Integer, BatchTotal> batchTotals = new HashMap<Integer, BatchTotal>();
batchTotals.put(99, createBatchTotal(1, Constants.CREDIT));
batchTotals.put(98, createBatchTotal(10, Constants.CREDIT));
batchTotals.put(97, createBatchTotal(5, Constants.DEBIT));
assertEquals(new BigDecimal(11), letterGenerator.creditBatchTotal(batchTotals, 1));
}
|
Still works! Wonderful. As I’m sure you’ve noticed, this test is already a bit shorter then the previous one was. As soon as I started writing the second test, I noticed there was a lot of code duplication happening, and made a few improvements. I moved the creation of the new ConfirmationLetterGenerator instance into the setUp method, after promoting the letterGenerator to a Field. And by creating a little createBatchTotal method, we can simplify further:
1
2
3
4
5
6
7
8
9
10
|
private BatchTotal createBatchTotal(int number, String sign) {
BatchTotal bt = new BatchTotal();
bt.setTransactionSign(sign);
if (Constants.CREDIT.equals(sign)) {
bt.setCreditValue(new BigDecimal(number));
} else if (Constants.DEBIT.equals(sign)) {
bt.setDebitValue(new BigDecimal(number));
}
return bt;
}
|
Still not happy, but it’s a start. Later refactorings should fix BatchTotal to hide some of this logic from us, but for now, we’ll continue. Note that I didn’t test that amountDivider thing. I’m pretty confident that a) BigDecimal.divide works, and b) we’ll be extracting that to a separate method pretty soon, so we can tackle testing it there.
In fact, let’s start removing some duplication in those batchTotal methods. Let’s see… They’re really the same, except for the number they get from the BatchTotal object. One gets the creditValue, the other gets the creditCounterValueForDebit. Unfortunately, it is completely unclear how these are filled, and what the determining rules are for when to use them. One could hypothesize that the transactionSign is the thing determining whether we’re working with a credit or debit transaction, but I can’t find any logic determining why in that case the debitValue is not used, but the creditCounterValueForDebit. No tests, and no documentation. Looking through the code, I do find the place where the BatchTotal objects are created, and it turns out that indeed the sign of the source record is the determining factor. But the transactionSign in BatchTotal is never filled (it doesn’t seem to be used anywhere, except in my just created, already out-of-date tests…)
So let’s just use what we do know. We know that the BatchTotal should give a total back, and which values to use for debit or credit totals. So let’s give BatchTotal a new method:
1
2
3
4
5
6
7
8
9
|
public BigDecimal getTotalForSign(String sign) {
BigDecimal value = null;
if (Constants.CREDIT.equals(sign)) {
value = getCreditValue();
} else if (Constants.DEBIT.equals(sign)) {
value = getCreditCounterValueForDebit();
}
return value;
}
|
Now again, this could be a lot better if I could depend on the sign within the BatchTotal class, but it seems like I can’t. This way, I can at least isolate the uncertainty. Which turns the two similar methods into:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
|
BigDecimal calculateTotalOverBatches(Map<Integer, BatchTotal> batchTotals,
Integer amountDivider, String sign) {
BigDecimal sum = BigDecimal.ZERO;
Iterator<BatchTotal> itr = batchTotals.values().iterator();
while (itr.hasNext()) {
BatchTotal total = itr.next();
sum = sum.add(total.getTotalForSign(sign));
}
return sum.divide(new BigDecimal(amountDivider));
}
BigDecimal creditBatchTotal(Map<Integer, BatchTotal> batchTotals,
Integer amountDivider) {
return calculateTotalOverBatches(batchTotals, amountDivider, Constants.CREDIT);
}
BigDecimal debitBatchTotal(Map<Integer, BatchTotal> batchTotals,
Integer amountDivider) {
return calculateTotalOverBatches(batchTotals, amountDivider, Constants.DEBIT);
}
|
The two separate methods can be removed, of course, as soon as I’ve moved the tests over to the new method, and changed the calls in createConfirmationLetter to the new call.
1
2
3
4
|
letter.setBatchTotalDebit(calculateTotalOverBatches(
hashBatchRecordsBalance.getBatchTotals(), client.getAmountDivider(), Constants.CREDIT).toString());
letter.setBatchTotalCredit(calculateTotalOverBatches(
hashBatchRecordsBalance.getBatchTotals(), client.getAmountDivider(), Constants.DEBIT).toString());
|
Since we have this working (I changed the tests, and removed the old methods, trust me!), but we can do better!
- accept just collection of BatchTotals in getBatchTotal, instead of Map
Because we don’t really need the keys, now do we?
- changed while in foreach in getBatchTotal
Which, apart from being prettier, also saves us some boilerplate code:
1
2
3
4
5
6
7
8
|
BigDecimal calculateTotalOverBatches(Collection<BatchTotal> batchTotals,
Integer amountDivider, String sign) {
BigDecimal sum = BigDecimal.ZERO;
for (BatchTotal batchTotal : batchTotals) {
sum = sum.add(batchTotal.getTotalForSign(sign));
}
return sum.divide(new BigDecimal(amountDivider));
}
|
But wait, what is this methods doing here anyway? We get a map from HashBatchRecordsBalance get stuff out of that map, calculate a total, and then put the result into another object. I don’t think that this functionality belongs here, so…
- moved getBatchTotal to HashBatchRecordsBalance
Very nice, we can even remove the collection from the method signature, since it’s already available locally in that object. If you don’t mind, I’ll leave this method for now. There’s some good stuff to dive into in the roughly 300 line calculateRetrieveAmounts method.
- Going on in calculateRetrieveAmounts: create isBalanced method to check client.getCounterTransfer().equalsIgnoreCase(Constants.TRUE)
It’s a big one, so we just start at the top. The first if is a great startingpoint. Doing a string compare to find out if a boolean value is set is not how we like to see things. We create a new method isBalanced in the Client class. The name comes from the comment further below (“Not Balanced”;), where the else for this conditional starts.
The nice thing about having a simple method for the logic for a conditional, is that you can test it separately. In this case, now that Client has a concept of isBalanced, I can test that is the preconditions are met, the expected boolean value is returned. In the situation where this is spread-out in the code somewhere, the only way to test it would be to look for the side-effects.
1
2
3
|
public boolean isBalanced() {
return Constants.TRUE.equalsIgnoreCase(getCounterTransfer());
}
|
1
2
3
4
5
6
7
8
9
10
11
12
13
14
|
public void testIsBalanced_TRUE() {
client.setCounterTransfer(Constants.TRUE);
assert(client.isBalanced());
}
public void testIsBalanced_FALSE() {
client.setCounterTransfer("FALSE");
assert(!client.isBalanced());
}
public void testIsBalanced_OTHER() {
client.setCounterTransfer("GERAG#$");
assert(!client.isBalanced());
}
|
I can hear some people go: “Pfff… Why test this. What is the use?”;. Well, at the very least it is documentation. The next person going through this code will be able to see quickly that a balanced client simply is a client with the counterTransfer set to ‘TRUE’, and that any other situation means it is not balanced. And that this was the intention of the programmer.
- Extracted check on FL Currency to separate method
- Extracted check on EURO Currency to separate method
Yes, there’s an awful lot of checking what currency code the record is on. And apparently the ‘FL’ currency has two varients, of which one is only used in a particular bank. Anyway, it’s easy to start cleaning up, one step at a time. So let’s extract that FL check into a nice separate method:
1
2
3
4
5
|
private boolean hasFlCurrency(Record record) {
Integer currencyCode = record.getCurrency().getCode();
return Constants.FL_CURRENCY_CODE.equals(currencyCode)
|| Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK.equals(currencyCode);
}
|
I did the same for the EUR currency code. Make sure to use your IDEs ’extract method’ refactoring, so that any other occurrences of this logic are also replaced.
- Extracted check on record being Debit to separate method
1
2
3
|
private boolean isDebitRecord(Record record) {
return Constants.DEBIT.equalsIgnoreCase(record.getSign());
}
|
- Replaced new BigDecimal(0) with refs to BigDecimal.ZERO
Why? Well, No need for all those objects, if you’re going to discard them anyway. Apart from that, it’s mostly cosmetic.
- Reversed BigDec.add so the total is the base (reduces chance on nullpointers)
What can I say. It irritated me.
- Moved retrievedAmounts update to outside the for loop
Well, the last update would have overwritten all the earlier ones anyway, but it’s still rather wasteful, and confusing, to do this within the loop.
- Moved isDebitRecord check one ‘if’ level up
That check was done for each of the inner ‘if’ statements, so moving it one level up saves us two spurious checks. Begone, Duplication! Begone!
- created getCurrencyCode method, should be replaced by enum version/conversion (or Currency should do this type of conversion using a private enum)
1
2
3
4
5
6
7
8
9
10
11
12
13
|
protected String getCurrencyByCode(Integer code) {
if (Constants.EUR_CURRENCY_CODE.equals(code)) {
return Constants.CURRENCY_EURO;
} else if (Constants.FL_CURRENCY_CODE.equals(code)
|| Constants.FL_CURRENCY_CODE_FOR_WEIRD_BANK.equals(code)) {
return Constants.CURRENCY_FL;
} else if (Constants.USD_CURRENCY_CODE.equals(code)) {
return Constants.CURRENCY_USD;
} else {
throw new IllegalArgumentException("Unknown currency code encountered");
}
}
|
Yes, quite ugly, but I’m not touching the Currency class today. You get that sometimes.
- removed currency specific ‘if’s and replaced with addAmountToTotal method that uses currency code as key
Actually, this was done in two steps. First we get rid of the separate currency specific work within the loop:
1
2
3
4
5
6
7
8
9
10
11
12
13
|
if (client.isBalanced()) {
for (Record record : records) {
if ((record.getFeeRecord() != 1) && isDebitRecord(record)) {
String currencyIsoCode = getCurrencyByCode(record.getCurrency().getCode());
BigDecimal previousValue = retrievedAmounts.get(currencyIsoCode);
if (previousValue == null) {
previousValue = BigDecimal.ZERO;
}
BigDecimal newValue = previousValue.add(record.getAmount());
retrievedAmounts.put(currencyIsoCode, newValue);
}
}
}
|
Which is an improvement, but we can get that code out of the loop by extracting a method:
1
2
3
4
5
6
7
|
if (client.isBalanced()) {
for (Record record : records) {
if ((record.getFeeRecord() != 1) && isDebitRecord(record)) {
addAmountToTotal(retrievedAmounts, record);
}
}
}
|
Though there’s more to be improved in the addAmountToTotal method, we’re leaving it alone for a bit. Diving too deeply can make you get the feeling that you’re not getting anywhere, and calculateRetrieveAmounts has so much more left to offer!
- Moved isCounterTransferRecord boolean logic to Record
This point is another example of logic in the wrong place. if I want to know if this is a counterTransferRecord, I should be able to ask the record and get a simple yes or no.
1
2
3
|
public boolean isCounterTransferRecord() {
return Integer.valueOf(1).equals(getIsCounterTransferRecord());
}
|
Note that this is the reverse of the contents of what was in the if statement. Apparently, it’s checking for **!**isCounterTransferRecord. Did you get that first read? Neither did I. In fact, I only noticed it when writing the test:
1
2
3
4
5
6
7
8
9
10
11
|
public void testIsCounterTranferRecordTrue() {
Record record = new Record();
record.setIsCounterTransferRecord(Integer.valueOf(1));
assert(record.isCounterTransferRecord());
}
public void testIsCounterTranferRecordNotTrue() {
Record record = new Record();
record.setIsCounterTransferRecord(Integer.valueOf(0));
assert(record.isCounterTransferRecord());
}
|
It felt a bit weird to have 1 be false, and 0 be true, so I re-checked, and found out about the inversion.
And then you read on to the next line, and whaddayano, exactly the same construction. But now for hasFee.
- Moved the getFee == 0 logic into Record.hasFee
No code for this. It really is exactly the same as above.
The next thing that happens is interesting, since it shows how much even the current state of the code has improved over the original. Looking at the code now shows a very obvious bug:
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
|
if (!record.isCounterTransferRecord() && !record.hasFee()) {
if ((hasFlCurrency(record))) {
if (isDebitRecord(record)) {
recordAmountDebitFL = recordAmountDebitFL.add(record.getAmount());
}
if (record.getSign().equalsIgnoreCase(Constants.CREDIT)) {
recordAmountCreditFL = recordAmountCreditFL.add(record.getAmount());
}
if (hasEurCurrency(record)) {
if (isDebitRecord(record)) {
recordAmountDebitEUR = recordAmountDebitEUR.add(record.getAmount());
}
if (record.getSign().equalsIgnoreCase(Constants.CREDIT)) {
recordAmountCreditEUR = recordAmountCreditEUR.add(record.getAmount());
}
}
}
}
if (record.getCurrency().getCode().equals(Constants.USD_CURRENCY_CODE)) {
if (isDebitRecord(record)) {
recordAmountDebitUSD = recordAmountDebitUSD.add(record.getAmount());
}
if (record.getSign().equalsIgnoreCase(Constants.CREDIT)) {
recordAmountCreditUSD = recordAmountCreditUSD.add(record.getAmount());
}
}
|
So… The EUR specific code is unreachable (because it’s within the FL specific code). And the US code is outside of the !isCounterTransferRecord and !hasFee if statement!
Better fix that, even though it changes functionality. Cleaning up code often brings little fixes like this.
- FOUND UNREACHABLE CODE (hasEuroCurrency in unbalanced case is *within* is hasFlCurrency)
Talking about balance, let’s also create a isCreditRecord method.
- created isCreditRecord method
Again, remember to use your IDEs refactoring tools, it really saves typing in these cases (and missed replacements…)
The next step is a little bigger than most. I was losing sight of which BigDecimal was which, and where to pass them. And this next section was going to make that painful. So I pre-empted that.
- Created RetrievedAmountsHolder inner class to structure calculated data results
1
2
3
4
5
6
7
8
9
10
11
12
13
14
|
/**
* Data holder class to structure RetrievedAmounts
*/
class RetrievedAmountsHolder {
BigDecimal recordAmount = BigDecimal.ZERO;
BigDecimal recordAmountDebit = BigDecimal.ZERO;
BigDecimal recordAmountCredit = BigDecimal.ZERO;
BigDecimal amountSansDebit = BigDecimal.ZERO;
BigDecimal amountSansCredit = BigDecimal.ZERO;
BigDecimal totalDebit = BigDecimal.ZERO;
BigDecimal totalCredit = BigDecimal.ZERO;
BigDecimal faultyAccRecordAmountCredit = BigDecimal.ZERO;
BigDecimal faultyAccRecordAmountDebit = BigDecimal.ZERO;
}
|
You might notice that there are no currency specific fields in there. Oh, and I cheated here: this version of the inner class is the one from the end of this refactoring, so it may contain some fields that we won’t use for a while yet.
- Simplified calculation of counterbalance records totals by working independently of currency
In other words, use the RetrievedAmountsHolder we just made, initialise a map using currency as key, and RetrievedAmountsHolders as values, and get rid of all the currency specific ifs. I’m finally starting to feel that I earned my Anti-IF-Campaign banner on my blog!
1
2
3
4
5
6
7
8
9
10
11
|
if (!record.isCounterTransferRecord() && !record.hasFee()) {
String currencyIsoCode = getCurrencyByCode(record.getCurrency().getCode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
if (isDebitRecord(record)) {
holder.recordAmountDebit = holder.recordAmountDebit.add(record.getAmount());
}
if (isCreditMethod(record)) {
holder.recordAmountCredit = holder.recordAmountCredit.add(record.getAmount());
}
}
|
- extracted balanced and counterbalanced records into their own methods
1
2
3
4
|
if (client.isBalanced()) {
calculateTotalsForBalancedRecords(records, retrievedAmounts);
} else {
calculateTotalsForCounterBalancedRecords(records, retrievedAmounts);
|
Note that I got rid of the ‘Not Balanced’ comment. It’s no longer needed, is it?
- extracted setting TempRecord sign to client sign to separate method
This is simply doing an Extract Method, so we get:
1
2
3
4
5
6
|
private void setTempRecordSignToClientSignIfUnset(TempRecord tempRecord, Client client) {
if (tempRecord.getSign() == null) {
String sign = client.getCreditDebit();
tempRecord.setSign(sign);
}
}
|
Now, the next bit of code had me a little puzzled, since the code setting the currency code to a default seems to be aimed to be able to perform the rest of the code contained in the for loop, but since it’s wrapped in the else, it would never be used. I decided it was a but, and removed the else.
That resolved, we can fix up the code there to make it currency independent.
- used retrievedAmountsHolder to get rid of currency specific handling for sans duplicates
- extracted sansDuplicates to own method
Since we’ve done this exercise before, I’ll just show the resulting method. I cheated a bit, and already made some supporting methods to support TempRecords in the same way as the regular Records. Unfortunately, these two very similar classes don’t have a common interface. That would be a good idea, I think, but I’m sticking mostly with the current class for now. The example is not supposed to grow too big, after all.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
|
private void calculateTotalsForSansDuplicateFaultRecords(Client client,
List<TempRecord> sansDuplicateFaultRecordsList, Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (TempRecord sansDupRec : sansDuplicateFaultRecordsList) {
setTempRecordSignToClientSignIfUnset(sansDupRec, client);
setTempRecordCurrencyCodeToClientIfUnset(client, sansDupRec);
String currencyIsoCode = getCurrencyByCode(sansDupRec.getCurrencycode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
if (isDebitRecord(sansDupRec)) {
holder.amountSansDebit = holder.amountSansDebit.add(new BigDecimal(sansDupRec.getAmount()));
}
if (isCreditMethod(sansDupRec)) {
holder.amountSansCredit = holder.amountSansCredit.add(new BigDecimal(sansDupRec.getAmount()));
}
}
}
|
- Moved isCreditRecord and isDebitRecord to Record
Yeah, I know. I just said I didn’t want to touch too many other classes, but these methods really belong in Record/TempRecord. I just don’t want to change the hierarchy at this moment.
- Collapsed totals calculations into a for loop, extracting calculation to separate (calculateTotal) method.
So the rest of our big method is calculating the totals based on the already gathered numbers. It does this (in exactly the same way) for each currency, once for Debit, and once for Credit records. That gives us 6 times the following code:
1
2
3
4
5
6
7
8
9
|
if (retrievedAccountNumberAmounts.get("FaultyAccDebitFL") != null
&& amountSansDebitFL != null) {
totalDebitFL = recordAmountDebitFL.add(amountSansDebitFL).subtract(
retrievedAccountNumberAmounts.get("FaultyAccDebitFL"));
} else if (amountSansDebitFL != null) {
totalDebitFL = recordAmountDebitFL.add(amountSansDebitFL);
} else {
totalDebitFL = recordAmountDebitFL;
}
|
This is a fine example of what happens if you create your code so that it can return null values. This code is meant to implement the simple calculation total = X + Y - Z. The it/else if/else is there only to take care of possible null values. Oddly enough, even before our changes, all the variables used in the calculation were initialised to new BigDecimals with value ``. So…
1
2
|
totalDebitFL = recordAmountDebitFL.add(amountSansDebitFL)
.subtract(retrievedAccountNumberAmounts.get("FaultyAccDebitFL"));
|
This would work just as well! But repeating one line of code 6 times is still ugly. Rewriting this to use our RetrievedAmountsHolder turns this into:
1
2
3
|
RetrievedAmountsHolder holder = retrievedAmounts.get(Constants.CURRENCY_FL);
holder.totalDebit = holder.recordAmountDebit.add(holder.amountSansDebit)
.subtract(retrievedAccountNumberAmounts.get("FaultyAccDebitFL"));
|
Which an Extract Method can extract, though it needs a little massaging (and adding the Credit version) to turn into:
1
2
3
4
5
6
7
8
|
private void calculateTotals(RetrievedAmountsHolder holder,
BigDecimal faultyAccountDebitAmount,
BigDecimal faultyAccountCreditAmount) {
holder.totalDebit = holder.recordAmountDebit.add(holder.amountSansDebit)
.subtract(faultyAccountDebitAmount);
holder.totalCredit = holder.recordAmountCredit.add(holder.amountSansCredit)
.subtract(faultyAccountCreditAmount);
}
|
But, we should still substract the debit from the credit to wrap this in one nice method:
1
2
3
4
5
6
7
8
9
10
|
private BigDecimal calculateTotals(RetrievedAmountsHolder holder,
BigDecimal faultyAccountDebitAmount,
BigDecimal faultyAccountCreditAmount) {
holder.totalDebit = holder.recordAmountDebit.add(holder.amountSansDebit)
.subtract(faultyAccountDebitAmount);
holder.totalCredit = holder.recordAmountCredit.add(holder.amountSansCredit)
.subtract(faultyAccountCreditAmount);
return holder.totalCredit.subtract(holder.totalDebit).abs();
}
|
A bit better, this, making the calling code look like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
|
Map<String, BigDecimal> retrievedAccountNumberAmounts
= calculateAmountsFaultyAccountNumber(faultyAccountNumberRecordList, client);
BigDecimal recordAmountFL = calculateTotals(retrievedAmounts.get(Constants.CURRENCY_FL),
retrievedAccountNumberAmounts.get("FaultyAccDebitFL"),
retrievedAccountNumberAmounts.get("FaultyAccCreditFL"));
BigDecimal recordAmountUSD = calculateTotals(retrievedAmounts.get(Constants.CURRENCY_USD),
retrievedAccountNumberAmounts.get("FaultyAccDebitUSD"),
retrievedAccountNumberAmounts.get("FaultyAccCreditUSD"));
BigDecimal recordAmountEUR = calculateTotals(retrievedAmounts.get(Constants.CURRENCY_EURO),
retrievedAccountNumberAmounts.get("FaultyAccDebitEUR"),
retrievedAccountNumberAmounts.get("FaultyAccCreditEUR"));
retrievedAmounts.get(Constants.CURRENCY_EURO).recordAmount = recordAmountEUR;
retrievedAmounts.get(Constants.CURRENCY_USD).recordAmount = recordAmountUSD;
retrievedAmounts.get(Constants.CURRENCY_FL).recordAmount = recordAmountFL;
|
But it would be so much nicer if we can make this a little more generic. But to do that, we have to change the calculateAmountsFaultyAccountNumber method to start using out RetrievedAmountsHolder. I’m not going to do that in small steps, since it is almost exactly the same code as what we just changed.
A small change worth mentioning in that while changing that method, I noticed that the arguments for the setTempRecordSignToClientSignIfUnset and setTempRecordCurrencyCodeToClientIfUnset were reversed, so I used my IDEs Change Method Signature refactoring to fix that.
The result looks as follows:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
|
private void calculateAmountsFaultyAccountNumber(
List<TempRecord> faultyAccountNumberRecordList,
Map<String, RetrievedAmountsHolder> retrievedAmounts,
Client client) {
for (TempRecord faultyAccountNumberRecord : faultyAccountNumberRecordList) {
setTempRecordSignToClientSignIfUnset(faultyAccountNumberRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(faultyAccountNumberRecord, client);
RetrievedAmountsHolder holder = retrievedAmounts.get(
getCurrencyByCode(faultyAccountNumberRecord.getCurrencycode()));
if (faultyAccountNumberRecord.isDebitRecord()) {
holder.faultyAccRecordAmountDebit = holder.faultyAccRecordAmountDebit.add(
new BigDecimal(faultyAccountNumberRecord.getAmount()));
}
if (faultyAccountNumberRecord.isCreditRecord()) {
holder.faultyAccRecordAmountCredit = holder.faultyAccRecordAmountCredit.add(
new BigDecimal(faultyAccountNumberRecord.getAmount()));
}
}
}
|
Looks kinda familiar, doesn’t it? Don’t worry, we’ll get to that after we fixed our calling method. That one, including the newly extracted calculateOverallTotals, looks like this now:
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
|
private Map<String, RetrievedAmountsHolder> calculateRetrievedAmounts(List<Record> records,
List<com.example.record.domain.FaultRecord> faultyRecords, Client client, FileExtension extension,
List<TempRecord> faultyAccountNumberRecordList, List<TempRecord> sansDuplicateFaultRecordsList) {
Map<String, RetrievedAmountsHolder> retrievedAmounts = new HashMap<String, RetrievedAmountsHolder>();
if (client.isBalanced()) {
calculateTotalsForBalancedRecords(records, retrievedAmounts);
} else {
calculateTotalsForCounterBalancedRecords(records, retrievedAmounts);
calculateTotalsForSansDuplicateFaultRecords(client, sansDuplicateFaultRecordsList, retrievedAmounts);
calculateTotalsForFaultyAccountNumberRecords(faultyAccountNumberRecordList, retrievedAmounts, client);
calculateOverallTotalsForAllCurrencies(retrievedAmounts);
}
return retrievedAmounts;
}
private void calculateOverallTotalsForAllCurrencies(Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (RetrievedAmountsHolder holder : retrievedAmounts.values()) {
calculateTotal(holder);
}
}
private void calculateTotal(RetrievedAmountsHolder holder) {
holder.totalDebit = holder.recordAmountDebit.add(holder.amountSansDebit)
.subtract(holder.faultyAccRecordAmountDebit);
holder.totalCredit = holder.recordAmountCredit.add(holder.amountSansCredit)
.subtract(holder.faultyAccRecordAmountCredit);
holder.recordAmount = holder.totalCredit.subtract(holder.totalDebit).abs();
}
|
It’s already quite an improvement over the 300 line behemoth we started with! But we’re not done. First of all, I neglected to change the place where this method was called, way up in the createConfirmationLetter method, if you remember. Oh, btw. I changed the name of the method to calculateRetrieve<strong>d</strong>Amounts.
The next thing I did was put all the calculateTotalsFor* methods together. Here they are, see if you can notice anything:
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
|
private void calculateTotalsForBalancedRecords(List<Record> records,
Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (Record record : records) {
if ((record.getFeeRecord() != 1) && record.isDebitRecord()) {
addAmountToTotal(retrievedAmounts, record);
}
}
}
private void calculateTotalsForCounterBalancedRecords(List<Record> records,
Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (Record record : records) {
if (!record.isCounterTransferRecord() && !record.hasFee()) {
String currencyIsoCode = getCurrencyByCode(record.getCurrency().getCode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
if (record.isDebitRecord()) {
holder.recordAmountDebit = holder.recordAmountDebit.add(record.getAmount());
}
if (record.isCreditMethod()) {
holder.recordAmountCredit = holder.recordAmountCredit.add(record.getAmount());
}
}
}
}
private void calculateTotalsForSansDuplicateFaultRecords(Client client,
List<TempRecord> sansDuplicateFaultRecordsList, Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (TempRecord sansDupRec : sansDuplicateFaultRecordsList) {
setTempRecordSignToClientSignIfUnset(sansDupRec, client);
setTempRecordCurrencyCodeToClientIfUnset(sansDupRec, client);
String currencyIsoCode = getCurrencyByCode(sansDupRec.getCurrencycode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
if (sansDupRec.isDebitRecord()) {
holder.amountSansDebit = holder.amountSansDebit.add(new BigDecimal(sansDupRec.getAmount()));
}
if (sansDupRec.isCreditRecord()) {
holder.amountSansCredit = holder.amountSansCredit.add(new BigDecimal(sansDupRec.getAmount()));
}
}
}
private void calculateTotalsForFaultyAccountNumberRecords(
List<TempRecord> faultyAccountNumberRecordList,
Map<String, RetrievedAmountsHolder> retrievedAmounts,
Client client) {
for (TempRecord faultyAccountNumberRecord : faultyAccountNumberRecordList) {
setTempRecordSignToClientSignIfUnset(faultyAccountNumberRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(faultyAccountNumberRecord, client);
RetrievedAmountsHolder holder = retrievedAmounts.get(
getCurrencyByCode(faultyAccountNumberRecord.getCurrencycode()));
if (faultyAccountNumberRecord.isDebitRecord()) {
holder.faultyAccRecordAmountDebit = holder.faultyAccRecordAmountDebit.add(
new BigDecimal(faultyAccountNumberRecord.getAmount()));
}
if (faultyAccountNumberRecord.isCreditRecord()) {
holder.faultyAccRecordAmountCredit = holder.faultyAccRecordAmountCredit.add(
new BigDecimal(faultyAccountNumberRecord.getAmount()));
}
}
}
|
In fact, I think I’ll in-line the addAmountToTotal method for the balanced records method. It made it more similar, even if that one still misses the debit/credit separation.
But can we make these calls more similar? After all, if we can make them mostly the same, we can get rid of some more code! I started with the calls for sansDuplicates and faultyAccountNumbers, since those looked most similar.
To get there, I first made their method signatures the same, including the names of the parameters. And I also sync’d the name of the local variable in the for-each loop. This made it clear that the only difference between the two was the field of RetrievedAmountsHolder the values ended up in. Hmm… Let’s see what happens if I change the RetrievedAmountsHolder structure to be composed of Maps with DEBIT and CREDIT as keys…
This seems to lead us in the right direction:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
|
private void calculateTotalsForCounterBalancedRecords(List<Record> records,
Map<String, RetrievedAmountsHolder> retrievedAmounts) {
for (Record record : records) {
if (!record.isCounterTransferRecord() && !record.hasFee()) {
String currencyIsoCode = getCurrencyByCode(record.getCurrency().getCode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
addAmountToSignedTotal(record, holder.recordAmounts);
}
}
}
private void addAmountToSignedTotal(Record record, HashMap<String, BigDecimal> amounts) {
amounts.put(record.getSign(),
amounts.get(record.getSign()).add(record.getAmount()));
}
|
At this point, the retrievedAmounts parameter starts to get silly, so we’ll promote that thing to a field. This needs a bit of clean-up: make sure to remove the parameter from all method signatures! After that, extract the retrieval of the correct RetrievedAmountsHolder for the record, and the method looks like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
|
private void calculateTotalsForCounterBalancedRecords(List<Record> records) {
for (Record record : records) {
if (!record.isCounterTransferRecord() && !record.hasFee()) {
addAmountToSignedTotal(record,
getHolderForRecord(record).recordAmounts);
}
}
}
private RetrievedAmountsHolder getHolderForRecord(Record record) {
String currencyIsoCode = getCurrencyByCode(record.getCurrency().getCode());
RetrievedAmountsHolder holder = retrievedAmounts.get(currencyIsoCode);
return holder;
}
|
We can then do the same thing for the methods dealing with TempRecords. Yes, we really need to unify those Record thingies. Later.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
|
private void calculateTotalsForSansDuplicateFaultRecords(
List<TempRecord> tempRecordList,
Client client) {
for (TempRecord tempRecord : tempRecordList) {
setTempRecordSignToClientSignIfUnset(tempRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(tempRecord, client);
addAmountToSignedTotal(tempRecord, getHolderForTempRecord(tempRecord).sansAmounts);
}
}
private void calculateTotalsForFaultyAccountNumberRecords(
List<TempRecord> tempRecordList,
Client client) {
for (TempRecord tempRecord : tempRecordList) {
setTempRecordSignToClientSignIfUnset(tempRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(tempRecord, client);
addAmountToSignedTotal(tempRecord, getHolderForTempRecord(tempRecord).faultyAccountRecordAmounts);
}
}
|
So now I was at a bit of an impasse. I couldn’t see an easy way to reduce the code duplication you see above. Luckily, the solution was (again) easy, once I started to explicitly look for a way get rid of it. The ReceivedAmountsHolder class/struct had been introduced to create a little structure, and limit the amount of parameters needed in many of the methods I’d been creating. But now that receivedAmounts was promoted to a field, there wasn’t really much reason to keep all the different amounts in one structure. So let’s get rid of that field, and replace it with (still currency sensitive) separate fields:
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
|
HashMap<String, BigDecimal> recordAmount = new HashMap<String, BigDecimal>();
class CreditDebitHolder {
HashMap<String, BigDecimal> creditValues = new HashMap<String, BigDecimal>();
HashMap<String, BigDecimal> debitValues = new HashMap<String, BigDecimal>();
public BigDecimal getValue(String sign, String currency) {
BigDecimal value = creditValues.get(currency);
if (Constants.DEBIT.equals(sign)) {
value = debitValues.get(currency);
}
if (value == null) {
value = BigDecimal.ZERO;
}
return value;
}
public void setValue(String currency, String sign, BigDecimal value) {
if (Constants.DEBIT.equals(sign)) {
debitValues.put(currency, value);
} else {
creditValues.put(currency, value);
}
}
}
CreditDebitHolder recordAmounts = new CreditDebitHolder();
CreditDebitHolder sansAmounts = new CreditDebitHolder();
CreditDebitHolder totalAmounts = new CreditDebitHolder();
CreditDebitHolder faultyAccountRecordAmounts = new CreditDebitHolder();
|
Not quite happy with that code, yet, but let’s first see what it does to our duplication issue.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
|
private void calculateTotalsForSansDuplicateFaultRecords(
List<TempRecord> tempRecordList,
Client client) {
for (TempRecord tempRecord : tempRecordList) {
setTempRecordSignToClientSignIfUnset(tempRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(tempRecord, client);
addAmountToSignedTotal(tempRecord, sansAmounts);
}
}
private void calculateTotalsForFaultyAccountNumberRecords(
List<TempRecord> tempRecordList,
Client client) {
for (TempRecord tempRecord : tempRecordList) {
setTempRecordSignToClientSignIfUnset(tempRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(tempRecord, client);
addAmountToSignedTotal(tempRecord, faultyAccountRecordAmounts);
}
}
|
Yup, that works, and easily turns into a single method with an extra parameter!
1
2
3
4
5
6
7
8
9
10
11
|
private void calculateTotalOverTempRecords(List<TempRecord> tempRecordList,
CreditDebitHolder amountsHolder,
Client client) {
for (TempRecord tempRecord : tempRecordList) {
setTempRecordSignToClientSignIfUnset(tempRecord, client);
setTempRecordCurrencyCodeToClientIfUnset(tempRecord, client);
addAmountToSignedTotal(tempRecord, amountsHolder);
}
}
|
Still, this Record vs. TempRecord thing is bothering me. I still have another copy of this method specifically for Records. Maybe it’s time to spread some of this code around! Let’s start with getting rid of that direct dependency on a DAO class, caused by the lookup of a default currency. I’ll move that out to an existing Util class, which has a number of other methods that are similar, and depends on a score of DAOs. Not ideal (it should reallu move to Client but a definite improvement.
- Moved getDefaultCurrencyForClient into Utils.java (breaking dependency on currencyDao)
And my current class really has a number of separate functions. It calculates some totals, and it creates a new ConfirmationLetter object. And it also can serialise the ConfirmationLetter to a PDF file.
- Extract calculations to new ConfirmationLetterTotalsCalculator class
That’s a little better. I’ll not show the full code here, I’ll put up the end-result later on. But in the process, I found I could remove two unused methods (hasEurCurrency and hasFlCurrency) and made the calculateRetrievedAmounts return the recordAmount Map so it could be called from the original class. Oh, and finally removed all the unused parameters from the call’s signature. Promoting client to a field also simplifies some method signatures.
- Introduce common interface for Record and TempRecord
The interface I introduced in very limited. In fact, it only supports the methods needed for this particular functionality, no more. But using it gets me one step closer to ridding myself of the duplication in the calculateTotals* methods. I’ve called it GenericRecord which is horrendous. But will do until we find out what else the interface can be good for.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
|
package com.example.domain;
import java.math.BigDecimal;
public interface GenericRecord {
boolean isCreditRecord();
boolean isDebitRecord();
public boolean isCounterTransferRecord();
public boolean hasFee();
public BigDecimal getAmountAsBigDecimal();
public Integer getCurrencyNumericCode();
public void setCurrencyNumericCode(Integer code);
public String getSign();
public void setSign(String sign);
}
|
- Finally get rid of duplication in calculateTotals* methods by passing in a filter strategy object
The next step is a little big, but I really thought I should try to remove some more duplication. The problem I had left was the specific filtering logic of the different types of records. I couldn’t consolidate the calculateTotals* methods without getting rid of those if statements in there! My solution for this was to create a little filtering Strategy pattern, so that I could simply pass-in the correct filter. Of course, for some of the Lists of Records, there was no filtering, which is why there’s a default implementation that simply has ’true’ as the return value (it’s an inclusion filter).
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
|
class RecordFilterStrategy {
boolean filter(GenericRecord record) {
return true;
}
}
RecordFilterStrategy sansAmountsFilter = new RecordFilterStrategy();
RecordFilterStrategy faultyAmountsFilter = new RecordFilterStrategy();
RecordFilterStrategy recordAmountsFilter = new RecordFilterStrategy() {
boolean filter(GenericRecord record) {
return record.isCounterTransferRecord() && !record.hasFee();
}
};
RecordFilterStrategy balancedFilter = new RecordFilterStrategy() {
boolean filter(GenericRecord record) {
return !record.hasFee() && record.isDebitRecord();
}
};
|
This turns the calculationTotals* methods into:
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
|
public HashMap<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, balancedFilter);
}
return calculateOverallTotalsForAllCurrencies();
}
private void calculateTotalsOverRecords(List<GenericRecord> records,
CreditDebitHolder amountsHolder,
RecordFilterStrategy filterCommand) {
for (GenericRecord record : records) {
if (filterCommand.filter(record)) {
addAmountToSignedTotal(record, amountsHolder);
}
}
}
|
- Move getCurrencyForCode to the Currency class
Currency related logic in a Currency class! Ingenious, eh?
- Get rid of ugly conditional code in CreditDebitHolder
It would be remiss of me to get rid of so many ifs, and then introduce a bunch of new ones! Adding an extra HashMap to distinguish between the CREDIT and DEBIT signs allows us to get rid of most of those ifs. There’s one left, to ensure we don’t return any null values, but I don’t see a nice way to get rid of that one.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
|
class CreditDebitHolder {
HashMap<String, HashMap<String, BigDecimal>> values = new HashMap<String, HashMap<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);
}
}
|
I’m nearing the end of this post. I hope. But if you’ve been paying attention, you’ll have seen that there is a part of the original ConfirmationLetterGenerator class that we’ve not touched at all. I actually started working on that, the amountsAndRecords and getAmountsAndRecords methods, when I noticed that these methods were not used anywhere! They’re private, and unused. And maybe they were in use before. Or maybe they are there to be used in the future. I don’t think we’ll ever know. But they’re gone now:-)
This leaves us, at the end of this (way too long) post, with two classes. One, the ConfirmationLetterGenerator that creates a confirmation letter, and can serialize the letter to a pdf. Yes, that’s two responsibilities, and the creation of the ConfirmationLetter object could be moved to a separate factory class. Also, the way the letter method works is not very transparent, since it depends on the side effect of a value being put into a web context. I’ve changed the code so that you can easily use this functionality without resorting such close coupling with the web front-end, but kept the method (marked as deprecated) since it would be hard to change its use without doing further restructuring of the surrounding program, which was not my goal.
The other class, ConfirmationLetterTotalsCalculator, only calculates some totals, and is drastically reduced in size compared to the methods that did this work in our starting code. An interesting comparison (again with a nod to the Anti-If-Campaign) is that the original class had 58 if statements (15 of which were in the deleted ‘AmountsAndRecords’ code). The new ConfirmationLetterGenerator has 4, while the ConfirmationLetterTotalsCalculator class has 5.
In Sonar, the classes now have complexity scores of, respectively, 5.0 and 7.7 (coming from 98, if you remember from the start of this post):
The LCOM4 value of both is 1. Unfortunately, I didn’t make a screenshot of the LCOM4 values of the before situation, but you can imagine what they were like.
To finish this off, I’ll list the final code for these two classes here:
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
|
package com.example.confirmationletter;
import java.math.BigDecimal;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import org.springframework.webflow.execution.RequestContext;
import com.example.domain.AmountAndRecordsPerBank;
import com.example.domain.Client;
import com.example.domain.ConfirmationLetter;
import com.example.domain.GenericRecord;
import com.example.domain.HashBatchRecordsBalance;
import com.example.domain.Record;
import com.example.record.command.FileUploadCommand;
import com.example.record.domain.TempRecord;
import com.example.record.parser.FileExtension;
import com.example.record.service.impl.Constants;
public class ConfirmationLetterGenerator {
private LetterSelector letterSelector;
/**
* Method kept for compatibility with surrounding code, but this should not
* be called directly, as it has side-effects: puts things in a Spring
* WebFlow context!
*
* @deprecated
*/
public OurOwnByteArrayOutputStream letter(RequestContext context, FileUploadCommand fileUploadCommand,
Client client, HashBatchRecordsBalance hashBatchRecordsBalance, String branchName,
List<AmountAndRecordsPerBank> bankMap, List<com.example.record.domain.FaultRecord> faultyRecords,
FileExtension extension, List<Record> records, List<TempRecord> faultyAccountNumberRecordList,
List<TempRecord> sansDuplicateFaultRecordsList) {
ConfirmationLetter letter = createConfirmationLetter(fileUploadCommand, client, hashBatchRecordsBalance,
branchName, bankMap, faultyRecords, extension, records, faultyAccountNumberRecordList,
sansDuplicateFaultRecordsList);
OurOwnByteArrayOutputStream arrayOutputStream = generateConfirmationLetterAsPDF(client, letter);
context.getConversationScope().asMap().put("dsbByteArrayOutputStream", arrayOutputStream);
return arrayOutputStream;
}
/**
* Returns the given ConfirmationLetter as PDF file in a byte array.
*
* @param creditDebit
* @param letter The confirmation letter to print to PDF
* @return the serialised letter as a byte[]
*/
public OurOwnByteArrayOutputStream generateConfirmationLetterAsPDF(
Client client, ConfirmationLetter letter) {
return letterSelector.generateLetter(client.getCreditDebit(), letter);
}
/**
* Creates a new {@link ConfirmationLetter} object, using the given
* parameters to fill it.
* <p>
* Totals are calculated and added to the Letter.
*/
public ConfirmationLetter createConfirmationLetter(
FileUploadCommand fileUploadCommand, Client client,
HashBatchRecordsBalance hashBatchRecordsBalance, String branchName,
List<AmountAndRecordsPerBank> bankMap,
List<com.example.record.domain.FaultRecord> faultyRecords,
FileExtension extension, List<Record> records,
List<TempRecord> faultyAccountNumberRecordList,
List<TempRecord> sansDuplicateFaultRecordsList) {
ConfirmationLetter letter = new ConfirmationLetter();
letter.setCurrency(records.get(0).getCurrency());
letter.setExtension(extension);
letter.setHashTotalCredit(hashBatchRecordsBalance.getHashTotalCredit().toString());
letter.setHashTotalDebit(hashBatchRecordsBalance.getHashTotalDebit().toString());
letter.setTotalProcessedRecords(hashBatchRecordsBalance.getRecordsTotal().toString());
letter.setTransferType(hashBatchRecordsBalance.getCollectionType());
letter.setBanks(bankMap);
letter.setCreditingErrors(faultyRecords);
letter.setClient(client);
letter.setBranchName(branchName);
ConfirmationLetterTotalsCalculator calculator
= new ConfirmationLetterTotalsCalculator();
Map<String, BigDecimal> recordAmount
= calculator.calculateRetrievedAmounts(
Collections.<GenericRecord>unmodifiableList(records),
Collections.<GenericRecord>unmodifiableList(faultyAccountNumberRecordList),
Collections.<GenericRecord>unmodifiableList(sansDuplicateFaultRecordsList),
client);
letter.setRetrievedAmountEur(recordAmount.get(Constants.CURRENCY_EURO));
letter.setRetrievedAmountFL(recordAmount.get(Constants.CURRENCY_FL));
letter.setRetrievedAmountUsd(recordAmount.get(Constants.CURRENCY_FL));
letter.setBatchTotalDebit(hashBatchRecordsBalance.calculateTotalOverBatches(client.getAmountDivider(),
Constants.CREDIT).toString());
letter.setBatchTotalCredit(hashBatchRecordsBalance.calculateTotalOverBatches(client.getAmountDivider(),
Constants.DEBIT).toString());
letter.setTransactionCost(getTransactionCost(fileUploadCommand, hashBatchRecordsBalance));
letter.setTotalRetrievedRecords(fileUploadCommand.getTotalRecords());
return letter;
}
public String getTransactionCost(FileUploadCommand fileUploadCommand,
HashBatchRecordsBalance hashBatchRecordsBalance) {
String transactionCost = "";
if (fileUploadCommand.hasFee()) {
transactionCost = hashBatchRecordsBalance.getTotalFee().toString();
}
return transactionCost;
}
public void setLetterSelector(LetterSelector letterSelector) {
this.letterSelector = letterSelector;
}
}
|
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);
}
}
}
|
Disappointment
That was an interesting exercise, wasn’t it? And we definitely saw great improvements: fewer ifs, cleaner code, shorter methods, hopefully more understandable (method) naming. All good! So we can be proud of ourselves. We’re done, right?
No. In fact, the code is still, regretfully, not up-to-par. Why? Well, we didn’t pay attention to testing! This was partly intentional: Remember that I said I really didn’t understand what the code was supposed to do? I didn’t, but while cleaning it I did understand what the new code was supposed to do. But I didn’t switch to letting testing guide me as soon as I could. That has resulted in better, shorter, and still pretty much untestable code.
We can fix that. See you next week? (follow-up posted on: http://www.lagerweij.com/2011/06/08/code-cleaning-how-tests-improve-code/)
