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 1 down vote favorite

I have a Spring @Service to asynchronously send emails. The part that seems to be a bit off to me is some code that is specific to our QA environment. We want to send and receive all emails through a test email account so that it does not impact live users if we work with real data imported to QA. I check for the environment and some settings in .properties files and adjust the outgoing email accordingly.

Is there a better way to handle this?

@Async
public void send(String email, String subject, String body) {

    if (email == null) {
        log.error("Not sending; null input: email");
        return;
    }
    try {
        MimeMessage message = this.mailSender.createMimeMessage();
        MimeMessageHelper helper = new MimeMessageHelper(message);
        helper.setFrom(mailSender.getUsername(), getFrom());
        helper.setReplyTo(mailSender.getUsername());
        helper.setSubject(subject);

        //HERE IS THE PART THAT'S IFFY:

        if ("QA".equalsIgnoreCase(EmailManager.getEnvironmentName())) {
            if (isTestMode()) {
                if (!email.equals(getTestEmailAddress())) {
                    helper.setTo(getTestEmailAddress());
                    body += "<br/>This email was originally intended for " + email +
                            " but was redirected to " + getTestEmailAddress() + " for testing.";
                    helper.setText(body, true); //HTML on
                }
            }
        }
        else {
            helper.setTo(email);
            helper.setText(body, true); //HTML on
        }

        if (message == null) {
            log.error("Email message to send is null.  Email={}, Subject={}", email, subject);
            log.error("Body:");
            log.error(body);
            return;
        }
        this.mailSender.send(message);
    }
    catch (MailException | UnsupportedEncodingException | MessagingException e) {
        log.error(e.toString());
        e.printStackTrace();
    }
}

private static String getFrom() {
    Locale defaultLocale = Locale.getDefault();
    ResourceBundle bundle = ResourceBundle.getBundle("messages", defaultLocale);
    return bundle.getString("label.project.name");
}

protected static String getTestEmailAddress() {
    return ResourceBundle.getBundle(AbstractEnvironment.ENVIRONMENT)
            .getString("testEmailAddress");
}

protected static Boolean isTestMode() {
    return Boolean.valueOf(ResourceBundle.getBundle(AbstractEnvironment.ENVIRONMENT)
                                   .getString("redirectAllEmailsToTestEmailAddress"));
}

@Autowired
private JavaMailSenderImpl mailSender;

//setter needed for Junit
public void setMailSender(JavaMailSenderImpl mailSender) {
    this.mailSender = mailSender;
}
share|improve this question

1 Answer 1

up vote 1 down vote

Use Spring's dependency injection more aggressively

You are right to be suspicious about hardcoded environment-specific logic. But you're in luck. This is one of the typical problems that Spring can help you solve.

One way to solve this is to create an interface for controlling the destination email address, and inject an implementation appropriate for the environment. For example:

interface OutgoingEmailController {
    void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body);
}

Implementation to inject in QA:

class QAOutgoingEmailController implements OutgoingEmailController {
    @Override
    public void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body) {
        if (isTestMode()) {
            if (!email.equals(getTestEmailAddress())) {
                helper.setTo(getTestEmailAddress());
                body += "<br/>This email was originally intended for " + email +
                        " but was redirected to " + getTestEmailAddress() + " for testing.";
                helper.setText(body, true); //HTML on
            }
        }
    }
}

Implementation to inject in other environments:

class CommonOutgoingEmailController implements OutgoingEmailController {
    @Override
    public void adjustOutgoingEmail(MimeMessageHelper helper, String email, String body) {
        helper.setTo(email);
        helper.setText(body, true);
    }
}

In your original code you will have an @Autowired OutgoingEmailController outgoingEmailController;, and the IFFY code replaced with outgoingEmailController.adjustOutgoingEmail(helper, email, body);.

Or something like that.

Other remarks

It's odd that you check for null email at the beginning but check for null body at the end. It would be better to move the check on the body further up to the beginning.

share|improve this answer
    
That looks much better! Is there a best practice to tell Spring to load the correct environment? (Spring profiles?) You're correct about the null check too. –  bphilipnyc May 17 at 14:44
    
I don't know. I think it all depends on the XML file you put on your classpath. –  janos♦ May 17 at 14:59
    
Hmm - not sure I'd want to go with this change until I know the best way for Spring to load the correct environment. Right now, it's ugly, but it works. –  bphilipnyc May 18 at 1:07

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.