current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

In a payment application a day before and at the same time with some payments some messages(email) needs to be sent. I have a DTO (called EscrowPayment, projected from some entity) from which I generate and send two similar `messages.

I look for advice specifically on the following issues with the code I ended up:

  • Copy-pasted code.
  • Type suffix in identifiers. eg: nominalAmountStr
  • Static utility methods formatInteger etc (Used application wide).
  • String constants in code. But because they are parameterized by position and not by name; if I extract them to a configuration file as they are, I wouldn't know in which order I should be giving parameters to String.format.

Here is the code; it is sanitized and translated, but issues are clearly identifiable:

public class EscrowPaymentMessageServiceImpl {

    private JavaMailSender mailSender;

    // .....

    @Override
    public void escrowPaymentWillBeDone(EscrowPayment escrowPayment) throws BusinessLayerException {
        try {
            String nominalAmountStr = formatInteger(new BigDecimal(escrowPayment.getNominal()));
            String paymentDateStr = dateFormat(escrowPayment.getPaymentDate(), "dd/MM/yyyy");
            String interestRateStr = format(escrowPayment.getInterestRate(), "0.00###");
            String paymentAmountStr = format(escrowPayment.getPaymentAmount());
            String messageText = String
                    .format("BLAH BLAH some security with ID %s of nominal value EUR %s "
                            + "at date %s BLAH your account with number 1234567890 will be debited to pay "
                            + "EUR %s coupon payment for %%%s interest.", 
                            escrowPayment.getSecurityId(), nominalStr, paymentDateStr, 
                            interestRateStr, paymentAmountStr);

            sendMessage(messageText);
        } catch (MessagingException e) {
            getLog().error("An error occured while sending the email:", e);
            throw new BusinessLayerException("EscrowPaymentMessage.MessageCouldNotBeSent", e);
        }
    }

    @Override
    public void escrowPaymentIsDone(EscrowPayment escrowPayment) throws BusinessLayerException {
        try {
            String nominalAmountStr = formatInteger(new BigDecimal(escrowPayment.getNominal()));
            String paymentDateStr = dateFormat(escrowPayment.getPaymentDate(), "dd/MM/yyyy");
            String paymentAmountStr = format(escrowPayment.getPaymentAmount());
            String messageText = String.format("BLAH BLAH at date %s the security with ID %s of nominal value EUR %s "
                            + "BLAH your account with number 1234567890 has been debited to pay EUR %s." ,
                            paymentDateStr, escrowPayment.getSecurityId(), nominalStr, paymentAmountStr);

            sendMessage(messageText);
        } catch (MessagingException e) {
            getLog().error("An error occured while sending the email:", e);
            throw new BusinessLayerException("EscrowPaymentMessage.MessageCouldNotBeSent", e);
        }
    }

    private void sendMessage(messageText) throws MessagingException {
        //....
    }

}
share|improve this question
    
There's not much I can see, is using a template system like StringTemplate an option? –  Bobby Jan 17 at 13:18
    
Thanks @Bobby. I checking it out, but introducing a new dependency would be a little too costly for me in this occasion. –  abuzittin gillifirca Jan 20 at 8:21
add comment

1 Answer

up vote 1 down vote accepted

This strikes me as an occasion where a custom method with positional format indexing may help (Formatter argument_index). It is a little-known feature of Java's String.format / Formatter class, that you can reference values by their position. Consider the following methods:

@Override
public static final String formatEscrowPayment(String format, EscrowPayment escrowPayment) {
        String nominalAmountStr = formatInteger(new BigDecimal(escrowPayment.getNominal()));
        String paymentDateStr = dateFormat(escrowPayment.getPaymentDate(), "dd/MM/yyyy");
        String interestRateStr = format(escrowPayment.getInterestRate(), "0.00###");
        String paymentAmountStr = format(escrowPayment.getPaymentAmount());
        return String.format(format, 
                        escrowPayment.getSecurityId(), // 1
                        nominalStr,                    // 2
                        paymentDateStr,                // 3
                        interestRateStr,               // 4
                        paymentAmountStr);             // 5

}

private static final void sendExcrowMessage(String template, ExcrowPayment escrow) throws BusinessLayerException {
    try {
        String messageText = formatEscrowPayment(template, escrow);
        sendMessage(messageText);
    } catch (MessagingException e) {
        getLog().error("An error occured while sending the email:", e);
        throw new BusinessLayerException("EscrowPaymentMessage.MessageCouldNotBeSent", e);
    }
}

@Override
public void escrowPaymentWillBeDone(EscrowPayment escrowPayment) throws BusinessLayerException {
    sendEscrowMessage("BLAH BLAH some security with ID %1$s of nominal value EUR %2$s "
                    + "at date %3$s BLAH your account with number 1234567890 will be debited to pay "
                    + "EUR %5$s coupon payment for %%%4$s interest.",
                      escrowPayment);

@Override
public void escrowPaymentIsDone(EscrowPayment escrowPayment) throws BusinessLayerException {
        sendEscrowMessage("BLAH BLAH at date %3$s the security with ID %1$s of nominal value EUR %2$s "
                        + "BLAH your account with number 1234567890 has been debited to pay EUR %2$s." ,
}

Note how the substitution values are handled by their positions.... the %1$s references the first format value as a string. The %3$s references the 3rd value. You can change the order of the positional fields without having to change the positions of the values in the actual format call. Have a look at the documentation

share|improve this answer
    
Thanks. This is what was I trying to do in the first place. But I forgot about argument indexes in format strings, and forgot that I forgot. But this is what SE is for. I'll wait for another day or two before accepting, in case someone comes up with another answer that I don't know that I don't know :). –  abuzittin gillifirca Jan 20 at 8:28
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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