Subscribe to Event form one service to another

Question

In my architecture I have service layer, and for my question two relevant classes in this layer:

1. UserManagementService
2. EmailSenderService

In first service there is method: UnsubscribeUser which unsubscribes the user from a mailing list, and after user is successfully unsubscribed it dispatches event UserUnsubscribedFromEmails.

In second service there is event handler which, after UserUnsubscribedFromEmails is dispatched, adds the email (of user who unsubscribed from mailing list) to Suppression list (black list) of our email delivery service.

I have following issue with my current implementation:

At the moment EmailSenderService subscribes to this event (UserUnsubscribedFromEmails) inside of the constructor of UserManagementService, which makes UserManagementService dependant on EmailSenderService and as far as my understanding goes, this should not be the case.

Here is code for UserManagementService:

public class UserManagementService : IUserManagementService
{

    /// <summary>
    ///     The subscription service.
    /// </summary>
    private readonly ISubscriptionService subscriptionService;

    /// <summary>
    /// The user service.
    /// </summary>
    private readonly IUserService userService;

    /// <summary>
    /// The email sender.
    /// </summary>
    private readonly IEmailSender emailSender;

    /// <summary>
    /// Initializes a new instance of the <see cref="UserManagementService"/> class.
    /// </summary>
    /// <param name="subscriptionService">
    /// The subscription service.
    /// </param>
    /// <param name="userService">
    /// The user Service.
    /// </param>
    /// <param name="emailSender">
    /// Email sender
    /// </param>
    public UserManagementService(ISubscriptionService subscriptionService, IUserService userService, IEmailSender emailSender)
    {
        this.subscriptionService = subscriptionService;
        this.userService = userService;
        this.emailSender = emailSender;

        // should this be here?
        this.UserUnsubscribedFromEmails += this.emailSender.OnUserUnsubscribedFromEmailsEventHandler;
    }


    /// <summary>
    /// User unsubscribed from emails event
    /// </summary>
    public event EventHandler<UserUnsubscribedFromEmailsEventArgs> UserUnsubscribedFromEmails;

    /// <summary>
    /// Unsubscribe user from mailing list
    /// </summary>
    /// <param name="userId">
    /// The user id.
    /// </param>
    /// <returns>
    /// The <see cref="Task"/>.
    /// </returns>
    /// <exception cref="ArgumentNullException">
    /// Throws if user id is null or empty
    /// </exception>
    /// <exception cref="InvalidOperationException">
    /// Throws if user could not be found
    /// </exception>
    public async Task<bool> UnsubscribeUser(string userId)
    {
        if (string.IsNullOrEmpty(userId) || string.IsNullOrWhiteSpace(userId))
        {
            throw new ArgumentNullException(nameof(userId));
        }

        using (var db = new DatabaseContext())
        {
            var user = db.Users.Find(userId);

            if (user == null)
            {
                throw new InvalidOperationException("Could not find user");
            }

            if (user.IsSubscribedToEmails)
            {
                user.IsSubscribedToEmails = false;
                await db.SaveChangesAsync();

                // Fires UserUnsubscribedFromEmails event 
                OnUserUnsubscribedFromEmails(new UserUnsubscribedFromEmailsEventArgs(user.Email, "Unsubscribed via portal"));

            }

            return true;
        }
    }

    /// <summary>
    /// User unsubscribed from emails
    /// </summary>
    /// <param name="args">
    /// The args.
    /// </param>
    protected virtual void OnUserUnsubscribedFromEmails(UserUnsubscribedFromEmailsEventArgs args)
    {
        if (UserUnsubscribedFromEmails != null)
        {
            UserUnsubscribedFromEmails(this, args);
        }
    }
}

And the relevant code for the EmailSenderService:

/// <summary>
///     The email sender.
/// </summary>
public class EmailSender : IEmailSender
{


    /// <summary>
    /// The on user unsubscribed from emails event handler.
    /// </summary>
    /// <param name="source">
    /// The source.
    /// </param>
    /// <param name="args">
    /// The args.
    /// </param>
    /// <exception cref="ArgumentNullException">
    /// Throws if arguments are null
    /// </exception>
    public void OnUserUnsubscribedFromEmailsEventHandler(object source, UserUnsubscribedFromEmailsEventArgs args)
    {
        if (args == null || string.IsNullOrEmpty(args.Email))
        {
            throw new ArgumentNullException(nameof(args));
        }

        this.AddEmailToSuppressionListAsync(new List<string> { args.Email }, args.Info);
    }    
}

So my question is where should I subscribe EmailSenderService to UserUnsubscribedFromEmails event?

UserManagementService is called in ASP.net Web API controller - should I inject both services in the controller and subscribe EmailSenderService there or there is some better design solution?

Thanks in advance!

Answer 1

in your UnsubscribeUser Method, I wouldn't worry about checking to see whether or not the user is actually subscribed or not, if this method is called I would simply just set that value to false, it shouldn't hurt anything if the user is already unsubscribed, and it may even keep track of the unsubscription for you if you keep a field that says last updated.

and then I would probably want to double check that the value is actually false by returning the opposite of the value in IsSubscribedToEmails

return !user.IsSubscribedToEmails;

like this

public async Task<bool> UnsubscribeUser(string userId)
{
    if (string.IsNullOrEmpty(userId) || string.IsNullOrWhiteSpace(userId))
    {
        throw new ArgumentNullException(nameof(userId));
    }

    using (var db = new DatabaseContext())
    {
        var user = db.Users.Find(userId);

        if (user == null)
        {
            throw new InvalidOperationException("Could not find user");
        }

        user.IsSubscribedToEmails = false;
        await db.SaveChangesAsync();

        // Fires UserUnsubscribedFromEmails event 
        OnUserUnsubscribedFromEmails(new UserUnsubscribedFromEmailsEventArgs(user.Email, "Unsubscribed via portal"));

        return !user.IsSubscribedToEmails;
    }
}

This way you can tell if the task actually performed the task or if there was an issue changing the value in the DB Context. if you just return true, you may not have actually changed the value but still received a true value from the method.

---

I am not certain that you could decouple these events, my thinking is that the mail sender should not care about the maintenance of the UnsubscribedFromEmails list/collection, it should only be worried about sending emails, if it is not sending emails to that list, then it shouldn't worry about them.

In the case where you want to send an email to let the user know that they have unsubscribed, it should be a "Mail" object sent to the mail sender. the mail sender should only be worried about sending mail and nothing more.

Answer 2

in your case I would try to avoid subscribing to events as this couples the projects and references and prevents the garbage collector to clean up the unused service. if you are creating new services per use (probably you do) you would have a list of services which stay in memory and GC cant collect them because of the event subscription.

Instead you can create an event service and subscribe through the event service. in the event service you would have a list of weak references to the subscribers. this would prevent objects being in the memory unnecessarily.

I would actually advise you to use PRISM event aggregator