Refactoring legacy java banking code

Question

Hey i have some legacy code i'm working on (It is open source so no problem in me sharing it!). I wanted to refactor some parts in the extractResult method since it got pretty repetitive. The *.*.* are extracted from XML, so there is not much to do. I realized that the creation of the Saldos and Values is pretty similar though that's why i extracted the code for its creation into respective methods (createSaldo and createValue). The only thing that doesn't match is the info.reserved field which is also a Saldo but doesn't take the infos from the same XML as the other Saldos (That is why the keys are different). You can check the old version of the code here. The PR there is also from me, which includes changes for the UPD(). If you want to check the old version of the code take a look at GVSaldoReq.

How would you further refactor this? And thanks for any inputs!

protected void extractResults(HBCIMsgStatus msgstatus, String header, int idx) {
    HashMap<String, String> result = msgstatus.getData();
    GVRSaldoReq.Info info = new GVRSaldoReq.Info();

    info.konto = new Konto();
    info.konto.country = result.get(header + ".KTV.KIK.country");
    info.konto.blz = result.get(header + ".KTV.KIK.blz");
    info.konto.number = result.get(header + ".KTV.number");
    info.konto.subnumber = result.get(header + ".KTV.subnumber");
    info.konto.bic = result.get(header + ".KTV.bic");
    info.konto.iban = result.get(header + ".KTV.iban");
    info.konto.type = result.get(header + ".kontobez");
    info.konto.curr = result.get(header + ".curr");
    passport.fillAccountInfo(info.konto);

    Map<String, String> resultHeader = getByPrefix(result, header);
    info.ready = createSaldo(getByPrefix(resultHeader, "booked"));

    Map<String, String> pendingMap = getByPrefix(resultHeader, "pending");
    Optional.ofNullable(pendingMap.get("CreditDebit")).ifPresent(creditDebit -> 
    info.unready = createSaldo(pendingMap));

    info.kredit = createValue(getByPrefix(resultHeader, "kredit"), false);
    info.available = createValue(getByPrefix(resultHeader, "curr"), false);
    info.used = createValue(getByPrefix(resultHeader, "value"), false);

    retrieveReservedBalanceInfoFromUPD().ifPresent(accountDataInfo -> {
        Map<String, String> reservedBalanceInfo = getByPrefix(accountDataInfo, "Balance.VOR");
        Optional.ofNullable(reservedBalanceInfo.get("Amount")).ifPresent(
            amount -> {
                amount = amount.replace(".", "").replace(",", ".");

                info.reserved = new Saldo();
                info.reserved.value = new Value(amount, reservedBalanceInfo.getOrDefault("Currency", "EUR"));

                Optional.ofNullable(reservedBalanceInfo.get("Date")).ifPresent(
                        date -> info.reserved.timestamp = HBCIUtils.string2DateISO(reservedBalanceInfo.get("Date"), "yyyyMMdd")
                );
            }
        );
    });

    ((GVRSaldoReq) (jobResult)).store(info);
}

private Value createValue(Map<String, String> data, boolean negative) {
    final var value = data.get("value");

    if (value == null) {
        return null;
    }

    final var currency = data.get("curr");
    final var valueObj = new Value(value, currency);
    if (negative) {
        valueObj.setValue(valueObj.getBigDecimalValue().negate());
    }
    return valueObj;
}

private Saldo createSaldo(Map<String, String> data) {
    final Saldo saldo = new Saldo();
    saldo.value = createValue(getByPrefix(data, "BTG"), "D".equals(data.get("CreditDebit")));
    saldo.timestamp = HBCIUtils.strings2DateTimeISO(data.get("date"), data.get("time"));
    return saldo;
}

private static Map<String, String> getByPrefix(Map<String, String> data, String keyPrefix) {
    return data.entrySet().stream()
        .filter(entry -> entry.getKey().startsWith(keyPrefix + "."))
        .collect(Collectors.toMap(entry -> entry.getKey().substring(keyPrefix.length() + 1), Map.Entry::getValue));
    }

private Optional<Map<String, String>> retrieveReservedBalanceInfoFromUPD() {
    return Optional.ofNullable(passport.getUPD().get("KInfo.accountdata")).map(accountData -> {
            final var reservedBalancePattern = Pattern.compile("(Balance\\.VOR\\..*?)=(.*?(?=;))");
            final var matcher = reservedBalancePattern.matcher(accountData);

            Map<String, String> reservedBalanceInfo = new HashMap<>();
            while (matcher.find()) {
                final var key = matcher.group(1);
                final var value = matcher.group(2);
                reservedBalanceInfo.put(key, value);
            }

            return reservedBalanceInfo;
        }
    );
}

Answer 1

Small Issues

Still saw an other point:

HashMap<String, String> result = msgstatus.getData();

private Konto configureKonto(String header, 
                         HashMap<String, String> result) {

Replace HashMap with Map. As the remainder: program against interfaces. Then later you may change the implementation to LinkedHashMap (order by insertion), or SortedMap/TreeMap (order by key).

XML Deserialisation

This code maps XML to a Map, does some conversions, and produces objects like Konto.

Using JAXB with annotations is unproblematic to convert the XML to a java DOM (Document Object Model) which mirrors the XML.

It is sufficiently flexible to ignore undefined attributes and elements in XML, or have extra fields in java.

If you have a different (or no) object hierarchy, you can use inheritance:

<bar><foo/><foo/><foo/></bar>

@XmlElement("bar")
class Bar{
    ...
    List<Bar> foos;
}
@XmlElement("foo")
class Foo { }

as

@XmlElement("bar")
class Baz extends Bar {
    ...
    List<Foo> foos;
}
class Bar {
     ...
}

You should start simple though.

Conversions

Can be done too.

For amounts you use String converted to programmer's notation with a decimal point.

Immediately convert to BigDecimal with a Locale.GERMANY.

That would clean up things.

How to implement JAXB

Try it in a separate project, with simplified XML, and writing the java classes yourself. Gaining experience with List and Map fields, child classes, conversion and more.

Using Test Driven Development, you can jard-code your java DOM object hierarchy and write it back to XML for comparison.

That is fun.

Answer 2

This are my thougths:

Split big methods

Your method extractResults() is rather long and already visually split up into sections doing different things. Eg. there is a large block configuring a newly creates Konto object. This could be moved to its own method:

  protected void extractResults(HBCIMsgStatus msgstatus, 
                              String header,
                                    int idx) {
  //..
      info.konto = configureKonto(header, result);
      passport.fillAccountInfo(info.konto);
  //..
}
  private Konto configureKonto(String header, 
                             HashMap<String, String> result) {
      Konto konto = new Konto();
      konto.country = result.get(header + ".KTV.KIK.country");
      konto.blz = result.get(header + ".KTV.KIK.blz");
      konto.number = result.get(header + ".KTV.number");
      konto.subnumber = result.get(header + ".KTV.subnumber");
      konto.bic = result.get(header + ".KTV.bic");
      konto.iban = result.get(header + ".KTV.iban");
      konto.type = result.get(header + ".kontobez");
      konto.curr = result.get(header + ".curr");
      return konto;
  }

This opens the opportunity to move this method into a new KontoConfiguratior class...

Extract lambda blocks to methods

When your lambdas grow to blocks you should extract them to separate methods. That makes the lambda itself easier to read:

  protected void extractResults(HBCIMsgStatus msgstatus, 
                              String header,
                                    int idx) {
  //..
      retrieveReservedBalanceInfoFromUPD().ifPresent(
              accountDataInfo -> processAccountData(accountDataInfo, info));
  //..
}

  private void processAccountData(Map<String, String> accountDataInfo, 
                                GVRSaldoReq.Info info) {
  //..
}

Unitize similar tasks

At some points you use Optional to deal with possible null values. You should do this all over your code in the same way. When doing so (along with the first point) your method createValue() would look like this:

  private Value createValue(Map<String, String> data, boolean negative) {
      return Optional.of(data.get("value"))
              .map(value -> convertValue(value, data, negative))
              .orElseGet(null);
  }


  private Value convertValue(String value, 
                             Map<String, String> data,
                             boolean negative) {
  //..
  }

^{Detour on null: Returning literal null is a really bad idea. It is a potential cause for a NullPointerException in the code of someone else. Usually it is done as an error signal and throwing an exception would be more appropriate. If it does not signal an error, a neutral element (a value of zero (0) in this case) might be a better choice to be returned.
But either way, this is a quite grievous design decision.}

The same applies to the style of variable declaration. Some of your local variables are declared final while others (including method parameters) are not although they also never change. I admit, that I don't like the var key word but when you go for it you should consequently use that all over your code.

It's part of the boy scout rule that this stuff is consistent when you finished your refactoring, at least within the methods you touched.

Read API docu to avoid complex solutions

In your createValue() method you check if the Value is negative to make it positive in that case. Instead of introducing branching here you could simply apply the abs() method from the BigDecimal class:

private Value convertValue(String value, 
                           Map<String, String> data,
                           boolean negative) {
  final var currency = data.get("curr");
  final var valueObj = new Value(value, currency);
  valueObj.setValue(valueObj.getBigDecimalValue().abs());
  return valueObj;
}

This also makes the parameter negative obsolete.

---

[update]

The flag is actually there to make the value negative, not to make the negative value positive. – Saltuk Kezer

In that case my prefered solution would be to modify the incoming String parameter value before passing it to the Value constructor like this:

final var sign = isNegative?"-":"";
final var valueObj = new Value( sign+value, currency);

omitting the access to the internal representation.

BTW: This access to the internal representation is a violation of the encapsulation - information hiding principle and an anti pattern called feature envy: you manipulate a property of the object from outside.