5

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?

Improve this question

5 Answers 5

Reset to default
18

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

12 Comments

No, I'm not ignoring it. Did you see where I talk about prompting the user to correct the bad email address? That's handling the exception.
No, since this is a bottom-level utility method, the exceptions should most definitely not be handled here. Leave it to the callers.
@Jon: No, his middle paragraph perfectly summarizes the way exceptions were intended to be used: iff the issue can be dealt with.
@Jon: I gave him a verbal description of how to handle it at a higher level. I'll add a code example.
@John Saunders, should I catch and rethrow a different exception in this method, or simply let the exception happen on its own?
|
6

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.

Improve this answer

Comments

4

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
}
Improve this answer

18 Comments

This is actually a lot harder than it seems. One of the best ways to check for a proper email address is to try to create a MailAddress and catch the exception.
You're saying he should just stop letting bad data be fed to this method? Sometimes that's not possible. Sometimes data is generated by human beings through user interfaces that are not under our control... and then we have to deal with malformed or inadequate arguments - somewhere... And if there is more than one place that calls this methoid, this method is the last line of defense, and the last opportunity to deal with them.
See here: stackoverflow.com/questions/201323/…
-1! I think this is a very bad solution, it duplicates functionality that is obviously already in the MailAddress constructor. And if the MailAddress constructor has a different validity check, this can lead to wrong code. Jon B's suggestion is much(!) better.
@Russ: if you duplicate functionality, you will easily introduce errors in your code when the code is changed later. If the mail validation changes later on one side, one will most likely forget to change the validation on the other side.
|
3

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.
  }
}
Improve this answer

Comments

0

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)

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.