Build a message string depending on the type of signature of an invoice

Question

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 ?

Answer 1

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.