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.