Send email just once per day

Question

I want to send an email just once per day. There is one button and any user can click that button. The email must not be sent twice. When the first user clicks the button, the email is sent. If some other user clicks the button, the email will not be sent again.

This is how I implemented, and it's working. There are some cases, once a month, when 2 email are sent, but I don't know why.

I am sure that there is more elegant way to resolve this problem. Please share with me your ideas.

protected void Send(string email, String allRecipients, String subject, String body)
{
    //Be sure that email is send just once:
    //Lock an object, and if its already locked just continue,
    if (Monitor.TryEnter(syncLock))
    {
        try
        {
            //Send email only if is not send already.
            //Check in DB if is send.
            //IsSend function return true if email is send already, else false.
            if (!IsSent())
            {
                SendEmails(email, allRecipients, subject, body);
            }
        }
        finally
        {
            Monitor.Exit(syncLock);
        }
    }
}

Function that sends email:

public static void SendEmails(string email, String allRecipients, String subject, String body)
{
    //Flag if email is sent or not:
    Boolean isEmailSend = false;

    SmtpClient client = new SmtpClient();
    //Message:
    MailMessage message = new MailMessage(email, allRecipients, subject, body);
    try
    {
        isEmailSend = true;
        client.Send(message);

    }
    catch (SmtpException e)
    {
        isEmailSend = false;
        throw new SmtpException(e.ToString());
    }
    catch (TimeoutException e)
    {
        //Problem: Not sure if is email send or not:
        isEmailSend = false;
        throw new TimeoutException("TIME OUT:" + e.ToString());
    }
    catch (Exception e)
    {
        //Problem: Not sure if is email send or not:
        isEmailSend = false;
        throw new Exception(e.ToString());
    }
    finally
    {
        if (isEmailSend)
        {
            //Update DB.
            //Set that email is send.
            //This update in DB that email for current day is send already.
            SetAsSend();
        }
    }
}

IsSent function

    //Check if meeting is sent today.
    public static bool IsSent()
    {
        string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
        bool isSent = false;

        const string CHECK_IF_SEND = "SELECT TOP 1 IsSent FROM MessageTable m WHERE m.date =@todayDate ";

        SqlConnection conn = new SqlConnection(connectionString);
        SqlCommand command = new SqlCommand(CHECK_IF_SEND, conn);
        command.Parameters.Add("@todayDate", DbType.Date).Value = DateTime.Today;

        try
        {
            conn.Open();
            Object result = command.ExecuteScalar();
            if (result != null)
            {
                isSent = (bool)result;
            }
        }
        finally
        {
            conn.Close();
            conn.Dispose();
        }
        return isSent;
    }

Answer 1

Couple of remarks:

• IsSent() smells like a property, not a method. It should be more descriptive as well: what object are you calling this on? It seems to indicate Email.IsSent but that's not what you do: your description makes it sound more like EmailManager.IsDailyEmailSent.

• Dispose your SmtpClient instance.

• Don't set isEmailSend to true before you actually send the email. You're already synchronizing using the monitor so that's not a reason. Set isEmailSent (notice the t instead of d to indicate past tense) to false from the get-go and set it to true after the sending inside the try block. Now you won't have to repeat the isEmailSent = false anymore either.

• Don't rethrow exceptions like that, you're going to lose the stacktrace. If all you do is rethrow it without logging or handling then you might as well not catch it in the first place.

Answer 2

Normally, you don't catch Exceptions to throw them again, it doesn't make sense, let them rise up, don't catch them here, I don't think it is your intention to stop the exceptions here, so don't.

Let the Exceptions bubble up.

---

If you are checking in a database to see if it is sent already you could save to the database with a dateSent column or something similar and then have your application check to see if there is a record with a dateSent value that day, if there isn't send the e-mail if there is don't send an e-mail

Answer 3

you should be using a couple of using statements in your code, then you can get rid of the try/finally statement completely because the closing/disposing of the connection and command are done automatically.

so this:

    string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
    bool isSent = false;

    const string CHECK_IF_SEND = "SELECT TOP 1 IsSent FROM MessageTable m WHERE m.date =@todayDate ";

    SqlConnection conn = new SqlConnection(connectionString);
    SqlCommand command = new SqlCommand(CHECK_IF_SEND, conn);
    command.Parameters.Add("@todayDate", DbType.Date).Value = DateTime.Today;

    try
    {
        conn.Open();
        Object result = command.ExecuteScalar();
        if (result != null)
        {
            isSent = (bool)result;
        }
    }
    finally
    {
        conn.Close();
        conn.Dispose();
    }
    return isSent;

should look like this

public static bool IsSent()
{
    string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
    bool isSent = true;

    const string CHECK_IF_SEND = "SELECT TOP 1 IsSent FROM MessageTable m WHERE m.date =@todayDate ";

    using (SqlConnection conn = new SqlConnection(connectionString))
    using (SqlCommand command = new SqlCommand(CHECK_IF_SEND, conn)
           {Parameters.Add("@todayDate", DbType.Date).Value = DateTime.Today}) 
    {
        conn.Open();
        Object result = command.ExecuteScalar();
        if (result != null)
        {
            isSent = (bool)result;
        }
        return isSent;
    }
}

and a lot of what is inside the using blocks is not necessary either (thanks @rolfl) so we can rewrite it like this:

public static bool IsSent()
{
    string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
    const string CHECK_IF_SEND = "SELECT TOP 1 IsSent FROM MessageTable m WHERE m.date =@todayDate ";

    using (SqlConnection conn = new SqlConnection(connectionString))
    using (SqlCommand command = new SqlCommand(CHECK_IF_SEND, conn)
           {Parameters.Add("@todayDate", DbType.Date).Value = DateTime.Today}) 
    {
        conn.Open();
        return command.ExecuteScalar() != null;
    }
}

You don't really need the result set in this method so you just want to know if it exists so we can just return whether it is null or not.

Answer 4

Flag variables are generally clumsy. Also, the exception handling in SendEmails is superfluous. This should work just as well:

public static void SendEmails(string email, string allRecipients, string subject, string body)
{
    SmtpClient client = new SmtpClient();
    MailMessage message = new MailMessage(email, allRecipients, subject, body);

    // May throw any Exception, such as SmtpException or TimeoutException
    client.Send(message);

    // Update DB to record that email has been sent.
    // This notes in DB that email for current day is sent already.
    SetAsSent();
}

If client.Send() fails with an exception, then control never reaches the call to SetAsSent() (which I've renamed from SetAsSend()), and the exception propagates to the caller.

I also don't like the fact that SendEmails() records the sending of the e-mail, but does not also do the corresponding check. It would be better to move the call to SetAsSent() into Send(), so that Send() does both the checking and the recording.

Alternatively, since the SendEmails() function is now so short, I would just roll the code into Send(), so that it's impossible to accidentally call the code to send mail without doing the check.

In addition, it would be better to be consistent in the non-capitalization of string.