2
\$\begingroup\$

The following method was meant to build a message depending on the type of signature of an invoice. It contains some if-else statements and code duplication.

The original method :

private String createSignatureMessage(InvoiceDTO invoiceDetails, int signatureType, Amount paid, Amount remaining) {
    List<Object> params = new ArrayList<Object>();
    params.add(getCanal());
    if(signatureType == 1){
        params.add(invoiceDetails.getInvoiceId());
        // amount
        params.add(FormattingUtils.formatAmount(invoiceDetails.getTotalAmount()));
        params.add(Consts.EURO);
        params.add(invoiceDetails.getDate());
        // paid
        params.add(FormattingUtils.formatAmount(paid));
        params.add(Consts.EURO);
        // remaining
        params.add(FormattingUtils.formatAmount(remaining));
        params.add(Consts.EURO);
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.simple.signature.ins"):config.getPropertyStr("message.simple.signature"), params.toArray());
    } else if(signatureType == 2){
        params.add(invoiceDetails.getInvoiceId());
        // amount
        params.add(FormattingUtils.formatAmount(invoiceDetails.getTotalAmount()));
        params.add(Consts.EURO);
        params.add(invoiceDetails.getDate());
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.first.signature.ins"):config.getPropertyStr("message.first.signature"), params.toArray());
    } else if(signatureType == 3){
        params.add(invoiceDetails.getInvoiceId());
        // amount
        params.add(FormattingUtils.formatAmount(invoiceDetails.getTotalAmount()));
        params.add(Consts.EURO);
        params.add(invoiceDetails.getDate());
        // paid
        params.add(FormattingUtils.formatAmount(paid));
        params.add(Consts.EURO);
        // remaining
        params.add(FormattingUtils.formatAmount(remaining));
        params.add(Consts.EURO);
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.second.signature.ins"):config.getPropertyStr("message.second.signature"), params.toArray());
    } else {
        return "";
    }
}

My attempt to refactor it using a command pattern :

interface Handler{
    String handle(InvoiceDTO invoiceDetails, Amount paid, Amount remaining);
}

private void addTotalAmount(InvoiceDTO invoiceDetails, List<Object> params) {
    params.add(invoiceDetails.getInvoiceId());
    params.add(FormattingUtils.formatAmount(invoiceDetails.getTotalAmount()));
    params.add(Consts.EURO);
    params.add(invoiceDetails.getDate());
}

private void addAmount(Amount amount, List<Object> params) {
    params.add(FormattingUtils.formatAmount(amount));
    params.add(Consts.EURO);
}

private void createMessageForLastSignature(List<Object> params, InvoiceDTO invoiceDetails, Amount paid, Amount remaining) {
    addTotalAmount(invoiceDetails, params);
    addAmount(paid, params);
    addAmount(remaining, params);
}

private String createSignatureMessage(InvoiceDTO invoiceDetails, int signatureType, Amount paid, Amount remaining) {
    List<Object> params = new ArrayList<>();
    params.add(getCanal());
    Map<Integer, Handler> commandMap = new HashMap<>();
    commandMap.put(1, (ordre, b, c) -> {
        createMessageForLastSignature(params, ordre, b, c);
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.simple.signature.ins"):config.getPropertyStr("message.simple.signature"), params.toArray());
    });
    commandMap.put(2, (ordre, b, c) -> {
        addTotalAmount(ordre, params);
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.first.signature.ins"):config.getPropertyStr("message.first.signature"), params.toArray());
    });
    commandMap.put(3, (ordre, b, c) -> {
        createMessageForLastSignature(params, ordre, b, c);
        return MessageFormat.format(InvoiceType.INS.equals(invoiceDetails.getType())?config.getPropertyStr("message.second.signature.ins"):config.getPropertyStr("message.second.signature"), params.toArray());
    });

    return commandMap.containsKey(signatureType) ? commandMap.get(signatureType).handle(invoiceDetails, paid, remaining) : "";
}

Can I have some feedback on the refactoring ?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Oct 26, 2022 at 10:20
  • 1
    \$\begingroup\$ Could you tell us what the signatureTypes 1, 2, and 3 represent? \$\endgroup\$ Oct 27, 2022 at 5:53

1 Answer 1

1
\$\begingroup\$

It's going in the right direction. You are trying to extract common code and then each branch has in its own "callback" as implementation of your own interface (single method)!

As a functional approach, this seems quite good.

You can still split it more.

  • Message.format is there 3 times. Make your interface return List<Object> rather than the final String. It also solves another small problem which is your params object being in the outer scope for your callbacks.
  • Creation of your commandMap should be a separate method and possibly cached.
  • Your lambda names :)

Another approach to consider is more object oriented where you create full implementation of your interface for each class. From cleanup code I see not much point, but from readability point of view it has some benefits. You create 3 classes, one for each implementation and you can name them so that it easily signals, what each implementations is for, and gives a better idea what it means when for example signatureType is 2. Also this signatureType can be another property of the interface and when you create map, you create your interface instances and associate them by this. Message.format can be possibly in parent abstract class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.