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

Application please send me an E-mail

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

Problem

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 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?

Solution

Some thoughts about the code itself.

  • In general method names recommended as Pascal Case so sendEmail should ideally be named SendEmail



  • 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 recipientAddresses)
    {
        SendEmail(
            bodyText,
            new MailAddress(sender),
            recipientAddresses.Select(p => new MailAddress(p)).ToList());
    }

    public void SendEmail(
        string bodyText,
        MailAddress sender, 
        List 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.


  ">
    
  


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 recipientAddresses)
    {
        SendEmail(
            bodyText,
            new MailAddress(sender),
            recipientAddresses.Select(p => new MailAddress(p)).ToList());
    }

    public void SendEmail(
        string bodyText,
        MailAddress sender, 
        List 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"
    );
}

Code Snippets

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);
            }
        }
    }
}
<system.net>
<mailSettings>
  <smtp from="<fromaddress>">
    <network host="mailexchanger_String" password="??" userName="??" port="25" enableSsl="true" defaultCredentials="false" />
  </smtp>
</mailSettings>
</system.net>
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
    }
}
using(var emailer = new Emailer())
{
    emailer.SendEmail(
        bodyText,
        sender,
        "sendtome"
    );
}

Context

StackExchange Code Review Q#62785, answer score: 11

Revisions (0)

No revisions yet.