3
\$\begingroup\$

I wanted to know if this is the best approach for the Single Responsibility Principle. The responsibility of this class is to remind users whose accounts are about to expire due to inactivity.

  • On the 30th day of inactivity, the user gets an email.
  • On the 45th day of inactivity, the user does not get an email; they get disabled.

public class AccountReminder
{

    private readonly IUnitOfWork _uow;
    private readonly ISendExpiringEmail _sendExpiringEmail;

    public AccountReminder(IUnitOfWork uow, ISendExpiringEmail sendExpiringEmail)
    {
        _uow = uow;
        _sendExpiringEmail = sendExpiringEmail;
    }

    public void NotifyUpcomingExpiringAccounts()
    {

        // Establish dates
        DateTime accountWarningDateTime = DateTime.Now.AddDays(-30).Date;
        DateTime expirationDateTime = DateTime.Now.AddDays(-45).Date;

        // Retrieve users whose accounts are either about to expire or have already expired
        var users = _uow.UserRepository
            .FindBy(element => (DbFunctions.TruncateTime(element.LastLoginDate) == accountWarningDateTime));

        // Send a reminder
        foreach (var user in users)
        {
            string htmlBody = "<html xmlns:v=\"urn:...xmlns=\"http://www.w3.org/TR/REC-html40\" >";
            htmlBody += "<head><meta http-equiv=...</head>";
            htmlBody += "<body>";

            //following line for only outlook to display image
            htmlBody += "<v:shape id=\>...</v:shape>";
            htmlBody += "<table width='100%'...</table>";
            htmlBody += "</body></html>";

            AlternateView htmlView = AlternateView.CreateAlternateViewFromString(htmlBody, null, MediaTypeNames.Text.Html);
            AlternateView plainView = AlternateView.CreateAlternateViewFromString(htmlBody, null, "text/plain");
            LinkedResource pic1 = new LinkedResource("images/Logo.jpg", MediaTypeNames.Image.Jpeg);
            pic1.ContentId = "Logo";
            htmlView.LinkedResources.Add(pic1);

            _sendExpiringEmail.SendTo(new[] { user.Email }, "EmailSubject", htmlView, plainView);

        }

        DisableExpiringAccounts(expirationDateTime);
    }

    private void DisableExpiringAccounts(DateTime expirationDateTime)
    {
        var expiredUsers = _uow.UserRepository.FindBy(element => element.Enabled && (DbFunctions.TruncateTime(element.LastLoginDate) <= expirationDateTime));

        foreach (var user in expiredUsers)
        {
            user.Enabled = false;
        }

        if (expiredUsers.Count > 0)
        {
            _uow.SaveChanges();
        }
    }

}
\$\endgroup\$
1
\$\begingroup\$

I'm going to go with no. There's at least two responsibilities in your description alone:

  • Determine accounts that are about to expire due to inactivity.

and

  • Notify users of a situation (in this case, the inactivity expiry).

And then in your code, you have a third:

  • Disable expired account.

I can potentially see the first and third items being in a class together. But user notifications lumped into this smells and likely should be separated.

\$\endgroup\$
1
\$\begingroup\$

This violates the Single Responsibility principal because it is doing two things:

  1. Sending an email to users (reminding them)

  2. Disabling expired accounts.

There's nothing terribly wrong with the main idea, which is to have a class responsible for notifying people of an upcoming expiration, and also encapsulating the logic of expiring old accounts. I'm just not sure they belong tied together in the same class. What if you want to send reminders without disabling expired accounts?

Maybe it's the name of the class, AccountReminder. Disabling someone's account has nothing to do with reminding them.

There is a third responsibility in this class, which is rendering the HTML output for the email message. Technically this should be separate as well. The HTML emails probably need a common header or footer, which can easily be accomplished by using a templating engine, like Razor templates (you don't need to use them only in conjunction with ASP.NET MVC).

I would split this into two classes:

  1. ExpiringAccountReminder - Just has one public method to send the notifications

    1. Internally, render the HTML email with something like Razor templates, which will give you much greater control over the results in a modular fashion. Creating common headers and footers is a snap, and gives your product a consistent look and feel in e-mail communication
  2. ExpiredAccountDisabler - Just has one public method that finds expired accounts and disables them.

\$\endgroup\$
  • \$\begingroup\$ Great, i gives me some direction. \$\endgroup\$ – DavidJS Nov 3 '14 at 19:38
  • \$\begingroup\$ This is a Console Application, i made a mistake by adding the Asp.Net MVC tag. Any alternative for Razor on a Console Application? \$\endgroup\$ – DavidJS Nov 3 '14 at 19:50
  • \$\begingroup\$ You don't need ASP.NET MVC to use Razor. Check out RazorEngine or you could try installing the Razor runtime assemblies via NuGet in Visual Studio. \$\endgroup\$ – Greg Burghardt Nov 3 '14 at 20:09
  • \$\begingroup\$ Speaking of which, check out this SO question How to use Razor View Engine in a console application?. \$\endgroup\$ – Greg Burghardt Nov 3 '14 at 20:11
0
\$\begingroup\$

In addition to the answers about SRP some more thoughts about your code.

Naming

Class names (interfaces too) should be made out of nouns. So instead of ISendExpiringEmail a better name would be IExpiringEmailSender.

var

var should be used if it is clearly seen what type it is, which isn't the case for

 var users = _uow.UserRepository
            .FindBy..  

reminder for loop

The adding up of the htmlBody variable shouldn't be done inside the loop, but outside. Also this can be a const, as it won't change.

\$\endgroup\$
  • \$\begingroup\$ The only disagreement I have with var is that it is generally expected to be used in LINQ queries, et al. \$\endgroup\$ – 410_Gone Aug 10 '15 at 14:26

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.