8
\$\begingroup\$

I created a New Event Emailer.

this takes some fun events from a nice SQL query and creates a little HTML message and sends it to all the subscribers of events, when new event types are added we need to be able to tell them that they need to add the events to their subscription if they want to be notified of them.

One issue is that if we send too many emails to the same domain in a short period of time we will get black listed from that domain, and that's not good.

so I coded this emailer to take the list of emails and email 10 at a time with a 6 second delay in the middle.

Is there anything that is inefficient or some edge case that I am missing?

class NewEventMailer //: IDisposable
{
#if DEBUG
    private static string _messageHeader = Resources.NewEventsEmailer_Development.HeaderMessage;
    private static string _logFileLocation = Resources.DebugStrings.LogFileLocation;
    private static string _SmtpServer = Resources.DebugStrings.SmtpServer;
    private static string _SmtpServerPort = Resources.DebugStrings.SmtpServerPort;
    private static string _emailSender = Resources.DebugStrings.EmailSender;
    private static string _newEventSubjectLine = Resources.NewEventsEmailer_Development.NewEventSubjectLine;
    private static string _defaultRecipient = Resources.NewEventsEmailer_Development.DefaultRecipient;
    private static string _NewEventSubjectLine = Resources.NewEventsEmailer_Development.NewEventSubjectLine;

#else
    private static string _messageHeader = Resources.NewEventsEmailer_Production.HeaderMessage;
    private static string _logFileLocation = Resources.ProductionStrings.LogFileLocation;
    private static string _SmtpServer = Resources.ProductionStrings.SmtpServer;
    private static string _SmtpServerPort = Resources.ProductionStrings.SmtpServerPort;
    private static string _emailSender = Resources.ProductionStrings.EmailSender;
    private static string newEventSubjectLine = Resources.NewEventsEmailer_Production.NewEventSubjectLine;
    private static string _NewEventSubjectLine = Resources.NewEventsEmailer_Production.NewEventSubjectLine;

    //Default Email set in Development Resource.
    private static string _defaultRecipient = Resources.NewEventsEmailer_Development.DefaultRecipient;

#endif
    private string _eventMailMessage;
    private List<string> _addresses;

    public NewEventMailer(DataTable newEventTable, List<string> addresses)
    {
        this._addresses = addresses;

        _eventMailMessage = EventMessage(newEventTable);
    }

    public string EventMessage(DataTable eventTable)
    {
        StringBuilder newMessage = new StringBuilder();
        newMessage.Append("<table style='text-align:left; width:750px'><tr><th colspan='2' style='text-align:left;'>" + _messageHeader + "</th></tr>");
        newMessage.Append("<tr><td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Name</td><td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Group</td></tr>");
        foreach (DataRow row in eventTable.Rows)
        {
            newMessage.Append("<tr><td>" + row.Field<string>("EventName") + "</td><td>" + row.Field<string>("EventGroup") + "</td></tr>");
        }
        newMessage.Append("</table>");

        return newMessage.ToString();
    }

    public void SendNewEventEmail()
    {
        int emailCounter = 0;

        MailMessage emailMessage = GetNewMailMessage(); //First Mail Message
        bool emailSent = false;
        foreach (var address in _addresses)
        {
            emailSent = false;
            var bccAddress = new MailAddress(address);
            emailMessage.Bcc.Add(bccAddress);

            emailCounter++;
            if (emailCounter % 10 == 0)
            {
                using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
                {
                    mailClient.Send(emailMessage);
                    Thread.Sleep(6000);
                    emailSent = true;
                    emailMessage = GetNewMailMessage();
                }
            }
        }
        if (emailSent == false)
        {
            using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
            {
                mailClient.Send(emailMessage);
            }
        }
    }

    public MailMessage GetNewMailMessage()
    {
        MailMessage emailMessage = new MailMessage(_emailSender, _defaultRecipient);
        emailMessage.IsBodyHtml = true;
        emailMessage.Body = _eventMailMessage;
        emailMessage.Subject = _NewEventSubjectLine;
        return emailMessage;
    }
}

And here is the Method in the main program class that calls everything into play

public static void SendNewEventsEmails()
{
    List<string> Addresses = new List<string>();
    //Get Email Addresses
    using (SqlConnection emailConn = new SqlConnection(Resources.DebugStrings.AttorneyEventCS))
    using (SqlCommand emails = new SqlCommand("[SQL_SELECT_STATEMENT_FOR_EMAIL_ADDRESSES]", emailConn))
    using (SqlDataAdapter emailsToData = new SqlDataAdapter())
    using (DataTable emailsTable = new DataTable())
    {
        emailConn.Open();
        emailsTable.Load(emails.ExecuteReader());
        foreach (DataRow email in emailsTable.Rows)
        {
            Addresses.Add(email.Field<string>("EmailTo"));
        }
    }

    //Send Email
    using (SqlConnection conn = new SqlConnection(Resources.ProductionStrings.AttorneyEventCS))
    using (SqlCommand newEventSproc = new SqlCommand()
    {
        CommandType = CommandType.StoredProcedure
        , CommandText = "[SPROC_Name]"
        , Connection = conn
    })
    {
        conn.Open();
        newEventSproc.Parameters.AddWithValue("@DaysPast", 30);
        DataTable newEventsTable = new DataTable();
        newEventsTable.Load(newEventSproc.ExecuteReader());
        NewEventMailer newEventMailer = new NewEventMailer(newEventsTable, Addresses);
        newEventMailer.SendNewEventEmail();
    }
}

Production has new events in the last 30 days, hence the reason for the production connection string, I still have some testing to do to get this to only email on the 1st of the month. but it works the way I intended it to.

\$\endgroup\$
6
\$\begingroup\$

Duplicated code

In SendNewEventEmail, there is some duplication in the email sending part:

// exhibit #1
using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
{
    mailClient.Send(emailMessage);
    Thread.Sleep(6000);
    emailSent = true;
    emailMessage = GetNewMailMessage();
}

And then again:

// exhibit #2
using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
{
    mailClient.Send(emailMessage);
}

It seems that it won't change your logic if you rewrite exhibit #1 like this:

using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
{
    mailClient.Send(emailMessage);
}
Thread.Sleep(6000);
emailSent = true;
emailMessage = GetNewMailMessage();

This you have the using (...) { ... } block exactly duplicated twice, so you can extract that to a helper method.

Odd emailSent variable

I looked with suspicion at the emailSent variable that gets set multiple times. If I'm looking at it right, its purpose is to know after the loop whether there are still a bunch of messages not yet sent. It seems you could get this same information from the emailCounter variable.

Suggested implementation

Putting the above points together, I think this function would be simpler and better this way:

public void SendNewEventEmail()
{
    int emailCounter = 0;
    MailMessage emailMessage = GetNewMailMessage();  // First Mail Message

    foreach (var address in _addresses)
    {
        var bccAddress = new MailAddress(address);
        emailMessage.Bcc.Add(bccAddress);

        emailCounter++;
        if (emailCounter % 10 == 0)
        {
            SendEmail(emailMessage);  // the new helper method (TODO)
            Thread.Sleep(6000);
            emailMessage = GetNewMailMessage();
        }
    }
    if (emailCounter % 10 > 0)
    {
        SendEmail(emailMessage);
    }
}

Single responsibility principle

The SendNewEventsEmails is doing too much:

  • Dig up email information from database
  • Prepare list of addresses and execute sending

It would be better to split these up:

  • A database accessor class should be in charge of loading data and returning it, and nothing else
  • An email sending class should not have to work with a database table and should not be aware of table schemas, such as the EventName and EventGroup column names and their types.

These two classes would be better to decouple from each other, they will become more generally reusable that way. There should be a third class that's driving the actions, using these two components. The result will be something where the components will be testable individually.

\$\endgroup\$
6
\$\begingroup\$

This part is jumping at me:

#if DEBUG
    private static string _messageHeader = Resources.NewEventsEmailer_Development.HeaderMessage;
    private static string _logFileLocation = Resources.DebugStrings.LogFileLocation;
    private static string _SmtpServer = Resources.DebugStrings.SmtpServer;
    private static string _SmtpServerPort = Resources.DebugStrings.SmtpServerPort;
    private static string _emailSender = Resources.DebugStrings.EmailSender;
    private static string _newEventSubjectLine = Resources.NewEventsEmailer_Development.NewEventSubjectLine;
    private static string _defaultRecipient = Resources.NewEventsEmailer_Development.DefaultRecipient;
    private static string _NewEventSubjectLine = Resources.NewEventsEmailer_Development.NewEventSubjectLine;

#else
    private static string _messageHeader = Resources.NewEventsEmailer_Production.HeaderMessage;
    private static string _logFileLocation = Resources.ProductionStrings.LogFileLocation;
    private static string _SmtpServer = Resources.ProductionStrings.SmtpServer;
    private static string _SmtpServerPort = Resources.ProductionStrings.SmtpServerPort;
    private static string _emailSender = Resources.ProductionStrings.EmailSender;
    private static string newEventSubjectLine = Resources.NewEventsEmailer_Production.NewEventSubjectLine;
    private static string _NewEventSubjectLine = Resources.NewEventsEmailer_Production.NewEventSubjectLine;

    //Default Email set in Development Resource.
    private static string _defaultRecipient = Resources.NewEventsEmailer_Development.DefaultRecipient;

#endif

Not because _SmtpServer, _SmtpServerPort and _NewEventSubjectLine aren't following the naming convention (they should be _smtpServer, _smtpServerPort and _newEventSubjectLine... although static fields ought to be in PascalCase)... but because I think it's a bit of an abuse of #if/#else/#endif directives.

It would probably be better handled with proper configuration settings; you can leverage pre- and post-build events to determine whether to build with app.config.debug or app.config.release.

Resource strings are meant for localization, not configuration.

\$\endgroup\$
  • \$\begingroup\$ I was changing lots of these variables at the last minute. some were copied in from another class. so what you are saying is that the Subject lines and the email addresses are a good fit here but the configuration stuff like _SmtpServer, _SmtpPort, _logFileLocation should be in a configuration file. I will be honest with you, I have not had much luck using configuration files to switch between running development and production, I made a mess of the subscription part of this application, so I will have to look this up some more. \$\endgroup\$ – Malachi Mar 11 '15 at 13:24
5
\$\begingroup\$

In addition to the valid points already mentioned

In this piece of code

if (emailCounter % 10 == 0)
{
    using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
    {
        mailClient.Send(emailMessage);
        Thread.Sleep(6000);
        emailSent = true;
        emailMessage = GetNewMailMessage();
    }
}  

the creation of the mailmessage by calling GetNewMailMessage() isn't needed, because you only need to clear the Bcc like emailMessage.Bcc.Clear() to remove any bcc addresses.


Instead of using a foreach loop together with a counter variable you should use a simple for loop and increment the counter by a step variable with the value 10. Inside the loop you then can call a method to add the next 10 addresses to the Bcc collection. After sending the message and calling Clear() on the said Bcc collection your code can safely sleep its time.

In this way you can remove the code duplication and also the checking for emailCounter % 10 == 0 and the omit the emailSent variable.

public void SendNewEventEmail()
{
    MailMessage emailMessage = GetNewMailMessage();
    int step = 10;
    for (int i = 0; i < _addresses.Count; i += step)
    {
        FillNextBccAddresses(emailMessage, i, step);
        using (SmtpClient mailClient = new SmtpClient(_SmtpServer, Convert.ToInt32(_SmtpServerPort)))
        {
            mailClient.Send(emailMessage);
            emailMessage.Bcc.Clear();
            Thread.Sleep(6000);
        }
    }
}

private void FillNextBccAddresses(MailMessage msg, int currentIndex, int amount)
{
    for (int i = 0; i < amount && (currentIndex + i < addresses.Count); i++)
    {
        msg.Bcc.Add(new MailAddress(_addresses[currentIndex + i]));
    }
}

Let us talk about this

public string EventMessage(DataTable eventTable)
{
    StringBuilder newMessage = new StringBuilder();
    newMessage.Append("<table style='text-align:left; width:750px'><tr><th colspan='2' style='text-align:left;'>" + _messageHeader + "</th></tr>");
    newMessage.Append("<tr><td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Name</td><td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Group</td></tr>");
    foreach (DataRow row in eventTable.Rows)
    {
        newMessage.Append("<tr><td>" + row.Field<string>("EventName") + "</td><td>" + row.Field<string>("EventGroup") + "</td></tr>");
    }
    newMessage.Append("</table>");

    return newMessage.ToString();
}  

Pro:

  • you are using a StringBuilder

Contra:

  • you are using a StringBuilder wrongly by using "+" to concatenate strings before you call .Append().
  • the name of the method isn't made out of a verb or a verb phrase.

So let us start by renaming the method to ComposeMessageContent() and use the StringBuilder in the right way

public string ComposeMessageContent(DataTable eventTable)
{
    StringBuilder newMessage = new StringBuilder();
    newMessage.Append("<table style='text-align:left; width:750px'>")
              .Append("<tr><th colspan='2' style='text-align:left;'>")
              .Append(_messageHeader)
              .Append("</th></tr>")
              .Append("<tr><td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Name</td>")
              .Append("<td style='border-bottom:groove; border-bottom-color:silver; font-size:x-large'>Event Group</td></tr>");

    foreach (DataRow row in eventTable.Rows)
    {
        newMessage.Append("<tr><td>")
                  .Append(row.Field<string>("EventName"))
                  .Append("</td><td>")
                  .Append(row.Field<string>("EventGroup"))
                  .Append("</td></tr>");
    }
    newMessage.Append("</table>");

    return newMessage.ToString();
}

Like @Janos already said this doesn't belong to the NewEventMailer class and should be extracted to another class which deals with database stuff. You can the pass it to the constructor as string.


Prefixing class member variables with ann underscore is ok, but because this already indicates that it is a class member variable you won't need to use this.

\$\endgroup\$
  • \$\begingroup\$ I was with you until you said to get rid of the foreach loop by using 2 for loops and an extra method. I just think that your code is going to be less efficient because we are calling _addresses list for the count and then recalling it every 10 emails and not calling it from memory. maybe I am wrong though because I am going to be holding that same list in memory throughout the entire foreach loop. \$\endgroup\$ – Malachi Mar 11 '15 at 13:29
  • \$\begingroup\$ you are so right about the string builder. thank you for pointing that out \$\endgroup\$ – Malachi Mar 11 '15 at 13:29
  • 1
    \$\begingroup\$ The _adresses is a List<string> which is already in memory. By using the for loop and incrementing the counter by 10 aka step this will iterate for 200 entries only 20 times. The other loop will iterate for each call of the method 10 times == 10 * 20 == 200 aka the same as before. \$\endgroup\$ – Heslacher Mar 11 '15 at 13:33

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.