HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpModerate

Send email just once per day

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
onceperemailjustsendday

Problem

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 Exce

Solution

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.

Context

StackExchange Code Review Q#61078, answer score: 15

Revisions (0)

No revisions yet.