Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
share|improve this question
1  
You don't need to both Close() and Dispose() your database connection object. Just Dispose() will do. Better yet, wrap it in a using construct. Also, same goes for the command as it implements IDisposable as well. –  Jesse C. Slicer Aug 26 at 14:23

4 Answers 4

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.

share|improve this answer
1  
MailMessage should be Dispose()d as well. –  Jesse C. Slicer Aug 26 at 13:49
1  
The finally statement is not required, OP could just call SetAsSend at the end of the try. You can also remove the boolean because of this. In fact, the whole try/catch is essentially useless here. –  Matthew Aug 26 at 14:43

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

share|improve this answer
    
In this case point is just to know what exception happened. Problem with exceptions here is that I do not know if email is send or not when exception happend. I know this only in SmtpException, but in case of TimeoutException(or some other exception) how to be sure if email is send or not? –  merso0027 Aug 26 at 13:36
1  
I am saying that you shouldn't catch the exception here, or you should write to a log file in the exception and not re-throw the exception –  Malachi Aug 26 at 13:45

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.