How should I handle exceptions in this C# function?

Question

I'm relatively new to C# and .NET and I'm trying to learn how to better handle exceptions in my code.

Take the following function I've written for example:

  public void SendEmail(string SenderEmail, string SenderDisplayName, IEnumerable<string> RecipientEmails, string Subject, string Message)
    {
        MailMessage message = new MailMessage();

        message.From = new MailAddress(SenderEmail, SenderDisplayName);
        foreach (var recipient in RecipientEmails)
        {
            message.To.Add(recipient);
        }
        message.Subject = Subject;
        message.Body = Message;

        SmtpClient smtpClient = new SmtpClient("192.168.168.182");
        smtpClient.Send(message);
    }
}

If you attempt to add an email address that is malformed in Message.From or Message.To, it will throw an exception. Right now my app is just crashing and burning when this happens.

Can someone show me the appropriate way to handle that exception in this method?

Answer 1

This is the appropriate way to handle exceptions!

In general, an exception should not be handled unless the problem can be corrected, and it should only be handled in a place where the correction can be applied.

For example, the caller of your code might want to prompt the user to correct the bad email address. But your code can't know the right way to prompt. Are you being called from WinForms or Web Forms? What should the dialog box look like? Should there even be a dialog box? These are things that can only be known by the caller of your method, and not by your method itself.

---

In the caller:

try
{
    SendEmail(SenderEmail, SenderDisplayName, RecipientEmails, Subject, Message);
}
catch (MyMailAddressException ex)
{
    MessageBox.Show(ex.Message);
}

Note that any exceptions other than MyMailAddressException will propagate to code that knows how to handle them.

---

Appropriate level of "handling" in your method:

public enum MailAddressType
{
    Sender,
    Recipient
}

public class MyMailAddressException : Exception
{
    public MailAddressType AddressType { get; set; }
    public string EmailAddress { get; set; }

    public MyMailAddressException(
        string message,
        MailAddressType addressType,
        string emailAddress,
        Exception innerException) : base(message, innerException)
    {
        AddressType = addressType;
        EmailAddress = emailAddress;
    }
}

public void SendEmail(
    string senderEmail,
    string senderDisplayName,
    IEnumerable<string> recipientEmails,
    string subject,
    string message)
{
    using (
        var mailMessage = new MailMessage
                          {
                              Subject = subject, 
                              Body = message
                          })
    {
        try
        {
            mailMessage.From = new MailAddress(
                senderEmail, senderDisplayName);
        }
        catch (FormatException ex)
        {
            throw new MyMailAddressException(
                "Invalid from address", MailAddressType.Sender,
                senderEmail, ex);
        }

        foreach (var recipient in recipientEmails)
        {
            try
            {
                mailMessage.To.Add(recipient);
            }
            catch (FormatException ex)
            {
                throw new MyMailAddressException(
                    "Invalid to address", MailAddressType.Recipient,
                    recipient, ex);
            }
        }

        var smtpClient = new SmtpClient("192.168.168.182");
        smtpClient.Send(mailMessage);
    }
}

The caller can then catch MyMailAddressException and have all the information necessary to tell the user what to fix. Other exceptions should propagate.

---

My previous edits have addressed your question about the method. I have been assuming that your application has appropriate top-level exception handling. Gabriel points out to me that if you had appropriate top-level exception handling, then your application would not be crashing!

However, crashing is not necessarily a bad thing. If something happens that your code cannot handle, then crashing is the right thing to do. The alternative is to try to continue running, hoping that this unhandled exception hasn't damaged your program in such a way that it begins to produce incorrect results.

The specifics of exactly where to put "top-level handlers" depends on your program. It's different between WinForms and ASP.NET applications, for instance. However, the concept will be the same: safely log all the available information, then allow the exception to propagate, crashing the application.

Of course, you should be using finally blocks to clean up your application, even in the presence of exceptions.

Answer 2

There are a number of possible solutions here, and which you choose is largely dependent upon what you want to happen.

What you need is a Try/Catch handler wrapped around the functionality in question in order to catch the thrown exception and deal with it.

Now, the thing is that you can wrap it all in an exception, and stop the email altogether, or you can do it on a per TO ADDRESS basis so that 1 bad address in a list of 100 wouldn't break the system. OR you could place it elsewhere (say where you call the function)

Answer 3

You shouldn't handle the exception. You should handle (sanitize) the input and make sure that the email addresses aren't malformed according to the requirements of the MailAddress Class

here is a very basic example:

public bool IsValidEmailAddress(string EmailAddress){
    Regex regEmail = new Regex(@"^[a-zA-Z0-9][\w\.-]*[a-zA-Z0-9]@[a-zA-Z0-9][\w\.-]*[a-zA-Z0-9]\.[a-zA-Z][a-zA-Z\.]*[a-zA-Z]$");

    if(regEmail.IsMatch(EmailAddress))
        return true;

    return false;
}

if (IsValidEmailAddress(SenderMail)){
    //do stuff to send the mail
}else{
    //return an error to the user interface
}

Answer 4

The best approach is to make sure that the input to SendMessage() is correctly formatted in a way that won't cause exceptions to be thrown in the first place. Do some validation and error checking.

In any case, though, if you were going to handle it, you wouldn't probably wouldn't handle it in SendMessage. Instead, go one level up:

public void Submit() {
  try {
    SendMessage(emailForm.SenderAddress, emailForm.UserDisplayName,
      emailForm.RecipientEmails, emailForm.Subject, emailForm.MessageBody);
  } catch (MailException e) { // Substitute whichever exception is appropriate to catch here.
    // Tell user that submission failed for specified reasons.
  }
}

Answer 5

Each method should only catch exceptions they can actually handle. I can't see how your SendMail method can possible do anything meaningful with an invalid mail address and thus it should just let the exception propagate to the caller.