1

I'm currently taking over a project that has a common pattern of interfaces like this:

public interface EmailService {

 void sendInvitationEmail(Payload payload);

 void sendNotificationEmailToAdmin(Payload payload);

 void sendPasswordResetEmailToUser(User user);

 void sendValidationEmailToNewUser(User user);
}

Then the implementation gets injected with many different services and repositories to collect the necessary relations that are needed to have every piece of information. I feel like this could be an interface with a single method:

public interface EmailService {
   void sendEmail(String recipient, EmailContext context);
}

But then I would often end up having multiple implementations injected into a single class that would send different types of emails, which also smells. Am I overthinking this? What's the preferred way to go about this?

  • 6
    You are looking at it backwards. Do the client code want to have one object with four methods or four objects, one for each type of email? What makes more sense from the client's point of view? – Goyo May 17 at 12:04
7

I'd be inclined to split the responsibilities into two: creating and sending emails, loosely coupled with an Email interface.

Then the EmailService takes the simple form you intuitively want and the implementation is only dependent on the transportation infrastructure.

public interface EmailService {
    void send(Email email) throws X;
}

Whilst an EmailFactory handles the creation of your various Email types.

public interface EmailFactory {
    Email createInvitation(Payload payload);
    Email createNotification(Payload payload);
    Email createPasswordReset(User user);
    Email createValidator(NewUser user);
}
  • This is a little better but it still ties up invitations, notifications and such with email. An email service should have nothing to do with content, it just gets a body. You do not bother telling your postal service to deliver an invitation, you have it deliver an envelope. It would be OK if you replaced Email with Message in the second interface. Then you would still need some sort of message router to direct the message into the appropriate channels. – Martin Maat May 26 at 8:12
4

In my opinion is the EmailService implementation is way to broad, it will be a great idea to split it into more specific implementations (like you did). the interface-segregation principle states:

ISP splits interfaces that are very large into smaller and more specific ones so that clients will only have to know about the methods that are of interest to them. Such shrunken interfaces are also called role interfaces.

So it's completely fine to have multiple implementations on a single class.

But why is a service implementation implemented by a repository?

  • I probably didn't explain it properly. The interface is not implemented by a repository, but multiple repositories get injected into the service implementation. Which makes ir a rather bloated class. – dvarsanyi May 17 at 8:07
  • 1
    The only responsibility a service or repository should have is interacting with a source. In my opinion it is a bad idea to inject repositories into a service, you basically give it another responsibility with every repo. Could a facade help for this issue? – Romano Schoonheim May 17 at 9:05
  • 1
    To be honest, I don't see why introducing an extra layer of abstraction would help in this case. I tend to avoid injecting repositories into controllers and such, but even this level of abstraction (repository -> service -> controller) seems to be tedious in most cases, because having tons of services delegate method calls to repositories without any added functionality doesn't really add anything to the project. However, introducing a simple pattern usually helps less experienced developers separate concerns and write testable code. This is a bit off topic, but something worth talking about:) – dvarsanyi May 17 at 9:31
  • 2
    @dvarsanyi I agree with you - and I also think that paying lip service to OOP principles is a bit of a waste of time since a "Service" class with a bunch of procedural logic in it is about as far from OOP as it gets. – Ant P May 17 at 13:27
1

If it was me, along with Alex answer here, I would have segregated interface into more granular level so that ISP (Interface Segregation Principle) is not violated.

public interface EmailFactory {
    Email createInvitation(Payload payload);
    Email createNotification(Payload payload);
    Email createPasswordReset(User user);
    Email createValidator(NewUser user);
}

Can be split into,

public interface PayloadEmailFactory {
    Email createInvitation(Payload payload);
    Email createNotification(Payload payload);
}

and

public interface UserEmailFactory {
    Email createPasswordReset(User user);
    Email createValidator(User user);
}
1

This is a classic scenario where it helps to separate the client API from the service provider interface (SPI).

Your SPI should be an interface that is easy for providers to implement, so it will likely have just one method that sends an email:

public interface EmailServiceSpi {
   void sendEmail(String recipient, EmailContext context);
}

Your client API can then be a concrete class that wraps the SPI and exposes higher-level methods that delegate to the SPI:

public class EmailService {
  private EmailServiceSpi spi;

  public void sendInvitationEmail(Payload payload);
  public void sendNotificationEmailToAdmin(Payload payload);
  public void sendPasswordResetEmailToUser(User user);
  public void sendValidationEmailToNewUser(User user);
}

The benefit of doing this is that you can continue to add features to the client API without having to change the provider interface every time.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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