views:

127

answers:

6

I have a Java EE project with Spring IoC container.

I've just found in Utils class static method sendMail(long list of params). I don't know why but I feel that it would look better if we had separate class (Spring bean with singleton scope) which will be responsible for sending email. But I can't find any arguments which can prove my position.

So, are there any pros (or cons) in using DI in this (rather general) situation?

+1  A: 

Util-type classes that hold a mishmash of usually static functions are not generally good things. I've even seen that kind of class named UglyGlobals in one project I worked on. At least they were honest about it! As a general rule, you are correct. Something like mailing is a great candidate for turning into a singleton bean that gets injected.

Tom Cabanski
UglyGlobals? Nice.
Platinum Azure
+5  A: 

If the classes that need to send email used a MailSender interface (sample name) instead of a static Utils.sendMail(...) method, then you can easily unit test those classes by swapping in a mock/different implementation of MailSender in your unit tests rather than use the real implementation.

This allows you to test the behavior of those classes when the mail sending succeeds, when an exception/error is caught, etc.

If the classes use a static method like what you've described, then you cannot unit test the behavior of these classes in isolation without actually sending email - at which point it is not an effective, isolated unit test since you will now be dependent on the behavior of 1) the mail-sending code 2) the outgoing mail server, etc.

matt b
+1. It's not my case (there are no unit tests in the project) but your argument is absolutely correct in general.
Roman
*there are no unit tests in the project* well, that should be a red flag. The use of static methods like this and the lack of DI likely makes unit testing nearly impossible.
matt b
+4  A: 

Yes! I'll give you an example right from my own recent experience.

We recently switched a bunch of apps from sending e-mail directly to using a database queue for sending messages. Messages are queued up in the DB then sent via a batch process. This gives us the ability to handle SMTP server outages, resend messages, control what messages can be sent from our dev server, verify that messages were actually sent, etc.

Something as simple as sending an e-mail can certainly be something you might want to change in the future. Injecting an implementation would make it much easier to do that.

Eric Petroelje
+1. I think it can be really changed sometimes in my project.
Roman
A: 

Spring Mail

First I would like to advise you to have a look at the org.springframework.mail package. It provides a helpful utility library for sending email and serves as abstraction to hide specifics of underlying mailing systems.

And since you are already using spring it would mean no pain to use this library as long as it provides everything you need to do with your application regarding sending mail.

Static Methods versus Dependency Injection

Using dependency injection gives you the possibility to easily switch to other implementations. Static method calls bind your implementation of a consumer tightly to the service that the consumer is using.

Testing the consumer means that you will integration test the consumer with the service instead of unit testing your consumer in isolation. This makes you always depend on the underlying logic of those static methods and makes tests possibly fail because of an error in one of those static methods.

codescape
A: 

DI makes it easy to mock your implementation for writing tests. For example, suppose you want to test a password reset flow. With a hard-coded Utils.sendMail(), your test code would be forced to make a mock SMTP server, read and parse the email and then click on the password reset link. If you had used DI, you could pass a mock Emailer object. That way, you can write super-fast unit tests, without bothering about external integrations.

You can also swap implementations easily. For example, Google App Engine has a custom sendMail API - so it would be difficult for you to support a GAE version of code and a non-GAE version at the same time. (For the sake of argument, just assume migrating to GAE is that easy. Its not, obviously).

Finally, your code is more modular. A particular class (ResetPassword) may just depend on Util.sendMail(), but Util could be a kitchen sink with methods to do everything under the sun. So, if you want to reuse ResetPassword in another project, you have to copy the entire Utils class, plus the several dependent jars that Utils needs to work. Its not a good option.

sri
The test cases angle has been already covered in other responses (I ought to type faster :), but the code modularity argument still applies
sri
A: 

You want to avoid static utility methods that depend on state or have external effects that you, in some situation (often testing), might want to avoid for any reason.

In this case, sending mail probably depends on external state (mail server availability) and produces external effects that you may well want to avoid (sending email). For development and testing, you probably do not generally want email to be sent at all. In other cases, you may want to test that an email has been sent, but how do you do this if mail is actually being sent? Set up some complicated system to check an actual email inbox for an email?

If you inject an interface representing a MailSender, you can provide a NoopMailSender that does nothing when you don't care about the email being sent and a StubMailSender fake that collects a List<EmailMessage> of emails that were sent through it when you want to be able to test that certain emails were sent out by your code.

ColinD