Application please send me an E-mail

Question

This is an E-mailer class that I created to send me E-mail when/if the Windows service (that will be installed on computers all over) throws an error, so that I know what is going on.

I use a similar set up all over the place, so I would like to know if this is clean and efficient.

class Emailer
{
    public void sendEmail(string bodyText, string sender, List<string> recipientAddresses)
    {
        int portNumber = 25;
        SmtpClient smtpClient = new SmtpClient("mailexchanger_String", portNumber);
        using (MailMessage mailMsg = new MailMessage())
        {
            mailMsg.Body = bodyText;
            mailMsg.IsBodyHtml = true;
            mailMsg.Priority = MailPriority.High; //I want to know now!
            foreach (string address in recipientAddresses)
            {
                mailMsg.To.Add(address);
            }
            mailMsg.From = new MailAddress(sender);

            smtpClient.Send(mailMsg);
        }
    }
}

I would also like to know if I have the right ideas about how classes should work and function; this is just a simple method wrapped inside of a class. Is this how it should be?

Answer 1

Some thoughts about the code itself.

1. In general method names recommended as Pascal Case so sendEmail should ideally be named SendEmail
2. I might consider trying to make the class a bit more configurable so that it could be extended in more ways without being modified. For example if you wanted to send a text email, or you wanted to send an email using a different server.

The modified code might look like:

public class Emailer
{
    // Required for emailer to work
    private readonly string _host;
    private readonly int _portNumber;

    // Optional configurable options
    public bool IsHtml { get; set; }
    public MailPriority Priorty { get; set; }

    public Emailer()
        : this("mailexchanger_String", 25)
    {

    }

    public Emailer(string host, int portNumber)
    {
        _host = host;
        _portNumber = portNumber;

        IsHtml = true;
        Priorty = MailPriority.High;
    }

    public void SendEmail(
        string bodyText,
        string sender,
        List<string> recipientAddresses)
    {
        SendEmail(
            bodyText,
            new MailAddress(sender),
            recipientAddresses.Select(p => new MailAddress(p)).ToList());
    }

    public void SendEmail(
        string bodyText,
        MailAddress sender, 
        List<MailAddress> recipientAddresses)
    {
        // .NET 4.0 only for SmtpClient disposing
        using (var smtpClient = new SmtpClient(_host, _portNumber))
        {
            using (var mailMsg = new MailMessage())
            {
                mailMsg.From = sender;
                mailMsg.Body = bodyText;
                mailMsg.IsBodyHtml = IsHtml;
                mailMsg.Priority = Priorty;                        
                recipientAddresses.ForEach(p => mailMsg.To.Add(p));

                smtpClient.Send(mailMsg);
            }
        }
    }
}

You can also configure an SMTP client via the .config. In that case you do not need to provide the connection details in the code but instead can configure it via the .config file.

For an MVC.net application that may be in the web.config or for a desktop application that might be in an app.config.

<system.net>
<mailSettings>
  <smtp from="<fromaddress>">
    <network host="mailexchanger_String" password="??" userName="??" port="25" enableSsl="true" defaultCredentials="false" />
  </smtp>
</mailSettings>
</system.net>

This means that you could create the SMTP client on Emailer creation and then dispose it as necessary. In this case the code might look like

internal class Emailer : IDisposable
{
    // Our smtp client that will be disposed
    private readonly SmtpClient _smtpClient;

    // Optional arguments
    private bool _isHtml = true;
    public bool IsHtml { get { return _isHtml;  } set { _isHtml = value; } }

    private MailPriority _mailPriority = MailPriority.High;
    public MailPriority Priorty { get { return _mailPriority;  } set { _mailPriority = value; } }

    public Emailer()
    {
        _smtpClient = new SmtpClient();
    }

    public Emailer(string host, int portNumber)
    {
        _smtpClient = new SmtpClient(host, portNumber);
    }

    public void SendEmail(
        string bodyText,
        string sender,
        List<string> recipientAddresses)
    {
        SendEmail(
            bodyText,
            new MailAddress(sender),
            recipientAddresses.Select(p => new MailAddress(p)).ToList());
    }

    public void SendEmail(
        string bodyText,
        MailAddress sender, 
        List<MailAddress> recipientAddresses)
    {
        using (var mailMsg = new MailMessage())
        {
            mailMsg.From = sender;
            mailMsg.Body = bodyText;
            mailMsg.IsBodyHtml = IsHtml;
            mailMsg.Priority = Priorty;                        
            recipientAddresses.ForEach(p => mailMsg.To.Add(p));

            _smtpClient.Send(mailMsg);
        }                
    }

    public void Dispose()
    {
        _smtpClient.Dispose(); // Or not if using < .NET 4.0
    }
}

And used like:

using(var emailer = new Emailer())
{
    emailer.SendEmail(
        bodyText,
        sender,
        "sendtome"
    );
}

Answer 2

Couple of points:

Naming & Typing

public void sendEmail(string bodyText, string sender, List<string> recipientAddresses)

• Method names in c# should be PascalCase, by convention.
• recipientAddresses does not need to be a List<string>; you're only using it to iterate its content, so IEnumerable<string> would be sufficient, and more flexible.
  • If, for whatever reason, the method would actually need a List<string>, it would be better to depend on an abstraction - here an IList<string>, or any ICollection<string> would be sufficient. Depending on the List<T> class is tying your code to a specific implementation of these interfaces.
• Because mailMsg.IsBodyHtml is true, the name bodyText is misleading - I'd call it bodyHtml to give the client code a clue about the formatting possibilities.

IDisposable

Since .net 4.0, the SmtpClient class implements the IDisposable interface, which means you should wrap it in a using block as well, unless you're targetting 3.5 or earlier.

---

A better approach might be to use a logging framework, such as NLog (available through NuGet). You can configure a log target to a SMTP server and have the exception's entire stack trace delivered to your inbox and logged to the application's log file, not to mention the ability to log TRACE, DEBUG, INFO or WARNING entries as well, and only send you an email (asynchronously!) when an ERROR entry is being logged.

---

You should also consider using SendAsync over Send, because, you know, async bests sync! :)