New event notification e-mailer

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.NewEventSubjectLi

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.

StackExchange Code Review Q#83801, answer score: 6