Make Email Sender more maintainable and testable

Question

I want to be able to improve the way I am sending these emails because it is currently not testable (via Mocking) and it is hard to maintain.

Main Sender (I also need an Async method on there but I have left it out):

public class MailSender
{
    protected bool SendEmail(MailAddress toAddress, string subject, string body)
    {
        var fromAddress = new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]);
        var fromPassword = WebConfigurationManager.AppSettings["FromPassword"];
        var smtpHost = WebConfigurationManager.AppSettings["SmtpHost"];
        var smtpPort = WebConfigurationManager.AppSettings["SmtpPort"];

        try
        {
            int port;
            Int32.TryParse(smtpPort, out port);

            var smtp = new SmtpClient
            {
                Host = smtpHost,
                Port = port,
                EnableSsl = true,
                DeliveryMethod = SmtpDeliveryMethod.Network,
                UseDefaultCredentials = false,
                Credentials = new NetworkCredential(fromAddress.Address, fromPassword)
            };
            using (var message = new MailMessage(fromAddress, toAddress)
            {
                Bcc = { new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]) },
                Subject = subject,
                Body = body,
                IsBodyHtml = true
            })
            {
                    smtp.Send(message);
            }
            return true;
        }
        catch (Exception)
        {
            return false;
        }
    }
}

The calling methods that build up the email content and subject:

    public bool SendEmailToCustomer(OrderDto order)
    {
        var subject = "A subject relating to this method";

        var body =
           File.ReadAllText(HttpContext.Current.Server.MapPath("/EmailTemplates/OrderAnswer.html"));
        var orderStatus = order.OrderStatus;
        body = body.Replace("#orderref#", order.OrderID.ToString());
        body = body.Replace("#orderstatus#", EnumHelper<OrderStatus>.GetDisplayValue(orderStatus));
        body = body.Replace("#questiontext#", order.Question);
        body = body.Replace("#answertext#", order.Answer);
        body = body.Replace("#Description#", order.OrderDescription);
        body = body.Replace("#BackgroundColour#", order.BackgroundColour.ToString());
        body = body.Replace("#DisplayText#", order.DisplayText);
        body = body.Replace("#Name#", order.Name);
        body = body.Replace("#HouseNumber#", order.HouseNumber);
        body = body.Replace("#FirstLineAddress#", order.FirstLineAddress);
        body = body.Replace("#SecondLineAddress#", order.SecondLineAddress);
        body = body.Replace("#ThirdLineAddress#", string.IsNullOrWhiteSpace(order.ThirdLineAddress) ? string.Empty : order.ThirdLineAddress);
        body = body.Replace("#City#", order.City);
        body = body.Replace("#PostCode#", order.PostCode);
        body = body.Replace("#ContactNumber#", order.ContactNumber);
        body = body.Replace("#email#", order.Email);
        if (SendEmail(new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]), subject, body))
        {
            return true;
        }
        return false;
    }

Now I have a few 'callers' like the method above (6 currently but could easily be more).

I have attempted to wrap interfaces around the Sender class and the callers but I'm just not progressing anywhere.

Anyone got any tips to lead me into the right direction?

OrderDto:

 public class OrderDto
{
    public int OrderID { get; set; }
    public string Email { get; set; }

    public string Name { get; set; }

    public DateTime? OrderDate { get; set; }

    public string HouseNumber { get; set; }

    public string FirstLineAddress { get; set; }

    public string SecondLineAddress { get; set; }

    public string ThirdLineAddress { get; set; }

    public string City { get; set; }

    public string PostCode { get; set; }

    public string ContactNumber { get; set; }

    public string OrderDescription { get; set; }

    public Colour BackgroundColour { get; set; }

    public string DisplayText { get; set; }
    public OrderStatus OrderStatus { get; set; }
    public string Question { get; set; }
    public string Answer { get; set; }
    public decimal? Price { get; set; }
    public bool CustomerPaid { get; set; }
}

OrderStatus:

    public enum OrderStatus
{
    [Display(Name = "Unknown")]
    Unknown = 0,
    [Display(Name = "New")]
    New = 1,
    [Display(Name = "Need More Information")]
    NeedMoreInformation = 2,
    [Display(Name = "Order Accepted")]
    OrderAccepted = 3,
    [Display(Name = "In Progress")]
    InProgress = 4,
    [Display(Name = "Completed")]
    Completed = 5,
    [Display(Name = "Shipped")]
    Shipped = 6
}

Answer 1

In SendEmailToCustomer() I think that it's suboptimal to perform that sequence of string.Replace() operations. It would be better to create a StringBuilder, walk through the string, detecting each "#" in turn and either adding what's in the string, or the proper thing.

Here's the first iteration:

public bool SendEmailToCustomer(OrderDto order)
{
    var subject = "A subject relating to this method";

    var body =
       File.ReadAllText(HttpContext.Current.Server.MapPath("/EmailTemplates/OrderAnswer.html"));
    var orderStatus = order.OrderStatus;
    var bodyPieces = bdp.Split('#');
    var bodyBuilder = new StringBuilder(body.Length);
    for(int bodyPieceIndex=0; bodyPieceIndex != bodyPieces.Length; bodyPieceIndex++)
    {
        bodyBuilder.Append(bodyPieces[bodyPieceIndex]);
        ++bodyPieceIndex;
        switch(bodyPieces[bodyPieceIndex]) {
        case "orderref":
            bodyBuilder.Append(order.OrderID.ToString());
            bodyPieceIndex++;
            break;
        case "orderstatus":
           bodyBuilder.Append(EnumHelper<OrderStatus>.GetDisplayValue(orderStatus));
            bodyPieceIndex++;
            break;

        case "questiontext":
            bodyBuilder.Append(order.Question);
            bodyPieceIndex++;
            break;

        case "answertext":
            bodyBuilder.Append(order.Answer);
            bodyPieceIndex++;
            break;

        case "Description":
            bodyBuilder.Append(order.OrderDescription);
            bodyPieceIndex++;
            break;

        case "BackgroundColour":
            bodyBuilder.Append(order.BackgroundColour.ToString());
            bodyPieceIndex++;
            break;

        case "DisplayText":
            bodyBuilder.Append(order.DisplayText);
            bodyPieceIndex++;
            break;

        case "Name":
            bodyBuilder.Append(order.Name);
            bodyPieceIndex++;
            break;

        case "HouseNumber":
            bodyBuilder.Append(order.HouseNumber);
            bodyPieceIndex++;
            break;

        case "FirstLineAddress":
            bodyBuilder.Append(order.FirstLineAddress);
            bodyPieceIndex++;
            break;

        case "SecondLineAddress":
            bodyBuilder.Append(order.SecondLineAddress);
            bodyPieceIndex++;
            break;

        case "ThirdLineAddress":
            if (!string.IsNullOrWhiteSpace(order.ThirdLineAddress)) bodyBuilder.Append(order.ThirdLineAddress);
            bodyPieceIndex++;
            break;

        case "City":
            bodyBuilder.Append(order.City);
            bodyPieceIndex++;
            break;

        case "PostCode":
            bodyBuilder.Append(order.PostCode);
            bodyPieceIndex++;
            break;

        case "ContactNumber":
            bodyBuilder.Append(order.ContactNumber);
            bodyPieceIndex++;
            break;

        case "email":
            bodyBuilder.Append(order.Email);
            bodyPieceIndex++;
            break;

        default:
            bodyBuilder.Append("#");
            break; // no increment
        }
    return SendEmail(new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]), subject, bodyBuilder.ToString());
}

Hmm. Still too much repeated code. Instead of a giant switch, how about a Dictionary instead?

public bool SendEmailToCustomer(OrderDto order)
{
    var subject = "A subject relating to this method";

    var body =
       File.ReadAllText(HttpContext.Current.Server.MapPath("/EmailTemplates/OrderAnswer.html"));
    var orderStatus = order.OrderStatus;
    var bodyPieces = bdp.Split('#');
    var bodyBuilder = new StringBuilder(body.Length);
    Dictionary<string,string> dict = new Dictionary {
        { "orderref", order.OrderID.ToString()) }
        { "orderstatus", EnumHelper<OrderStatus>.GetDisplayValue(orderStatus)) }
        { "questiontext", order.Question) }
        { "answertext", order.Answer) }
        { "Description", order.OrderDescription) }
        { "BackgroundColour", order.BackgroundColour.ToString()) }
        { "DisplayText", order.DisplayText) }
        { "Name", order.Name) }
        { "HouseNumber", order.HouseNumber) }
        { "FirstLineAddress", order.FirstLineAddress) }
        { "SecondLineAddress", order.SecondLineAddress) }
        { "ThirdLineAddress", string.IsNullOrWhiteSpace(order.ThirdLineAddress) ? string.Empty : order.ThirdLineAddress) }
        { "City", order.City) }
        { "PostCode", order.PostCode) }
        { "ContactNumber", order.ContactNumber) }
        { "email", order.Email) }
    };
    for(int bodyPieceIndex=0; bodyPieceIndex != bodyPieces.Length; bodyPieceIndex++)
    {
        bodyBuilder.Append(bodyPieces[bodyPieceIndex]);
        ++bodyPieceIndex;
        if (bodyPieceIndex == bodyPieces.Length) break;
        string replacement;
        if (dict.TryGetValue(bodyPieces[bodyPieceIndex],out replacement)
        {
            bodyBuilder.Append(replacement);
            bodyPieceIndex++;
        }
        else
            bodyBuilder.Append("#");
            // no increment
        }
    }
    return SendEmail(new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]), subject, bodyBuilder.ToString());
}

Well, doing some more research, we can save memory. StringBuilder.Append() has an overload that appends a section of another string without creating a temporary object. So, no more Split, just IndexOf:

public bool SendEmailToCustomer(OrderDto order)
{
    var subject = "A subject relating to this method";

    var body =
       File.ReadAllText(HttpContext.Current.Server.MapPath("/EmailTemplates/OrderAnswer.html"));
    var orderStatus = order.OrderStatus;
    int openingPound = body.IndexOf('#');
    int closingPound = -1;
    var bodyBuilder = new StringBuilder(body.Length);
    Dictionary<string,string> dict = new Dictionary {
        { "orderref", order.OrderID.ToString()) }
        { "orderstatus", EnumHelper<OrderStatus>.GetDisplayValue(orderStatus)) }
        { "questiontext", order.Question) }
        { "answertext", order.Answer) }
        { "Description", order.OrderDescription) }
        { "BackgroundColour", order.BackgroundColour.ToString()) }
        { "DisplayText", order.DisplayText) }
        { "Name", order.Name) }
        { "HouseNumber", order.HouseNumber) }
        { "FirstLineAddress", order.FirstLineAddress) }
        { "SecondLineAddress", order.SecondLineAddress) }
        { "ThirdLineAddress", string.IsNullOrWhiteSpace(order.ThirdLineAddress) ? string.Empty : order.ThirdLineAddress) }
        { "City", order.City) }
        { "PostCode", order.PostCode) }
        { "ContactNumber", order.ContactNumber) }
        { "email", order.Email) }
    };
    while (openingPound != -1)
    {
        // start out by appending text that's outside of a tag
        // here closingPound is the close of previous tag
        bodyBuilder.Append(body,closingPound+1,openingPound-closingPound+1);
        closingPound = body.IndexOf(openingPound+1,17);
        if (closingPound == -1)
        { // the pound was not part of a tag
            closingPound = openingPound-1;
            openingPound = body.IndexOf('#',openingPound+17);
            continue;
        }

        string replacement;
        string tag = body.Substring(openingPound+1,closingPound-openingPound-1);
        if (dict.TryGetValue(tag,out replacement)
        {
            bodyBuilder.Append(replacement);
            openingPound = body.IndexOf('#',closingPound+1);
        }
        else
        {
            closingPound = openingPound-1;
            openingPound = body.IndexOf('#',openingPound);
            continue;
        }
    }
    bodyBuilder.Append(body,closingPound+1,body.Length-closingPound+1);
    return SendEmail(new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]), subject, bodyBuilder.ToString());
}

Answer 2

I think your main obstacle is use of static methods/classes, like

WebConfigurationManager.AppSettings["FromEmail"]

There a two options to solve it:

1. Read all the values in your 'caller' method and pass them to your methods as parameters.
2. Create a wrapper class and expose it's interface.

Now, you don't really want to test the actual 'send' functionality, but rather the object construction.

So, I'd propose to create a builder classes for the following tasks:

1. Body construction from the template.
2. MailMessage construction.
3. SmtpClient construction.

Last 2 are pretty static (all they do is assign properties from configuration), so you won't gain much from covering them with tests (In my opinion).
As for the message body construction, I would create the following:

public interface IMailBodyBuilder
{
    string Build(string templateText, OrderDto order);
}
public class MailBodyBuilder
{
    public MailBodyBuilder(string templateText, OrderDto order)
    {

    }

    public string Build(string templateText, OrderDto order)
    {

    }
}

This way you'll be able to create a mock order and test the result in isolation.

And your 'caller' function would now look something like this:

 public bool SendEmailToCustomer(IMailBodyBuilder bodyBuilder, OrderDto order)
{
    var subject = "A subject relating to this method";

    var body =
       File.ReadAllText(HttpContext.Current.Server.MapPath("/EmailTemplates/OrderAnswer.html"));
   body = bodyBuilder.Build(body,order);
   return SendEmail(new MailAddress(WebConfigurationManager.AppSettings["FromEmail"]), subject, body))
}

Answer 3

I'd look into using a run-time text template. I used this with great success on a project to make my email sending logic testable.

I created a template called MyEmailTemplate (example name). In order to test the actual functionality of the template generation, I wrote unit tests for the class directly. I was able to accomplish this by keeping the methods stateless and free of side effects--just return a string based on the inputs. Once I got a layer up, I had to then figure out how to mock the template to test the layers dependent on email functionality.

The way I structured it, I had my DI container wire up an factory object I named IEmailTemplateFactory (I used Castle Windsor's TypedFactoryFacility, if you're interested) that then had a factory method that generated an instance of an IMyEmailTemplate. I would inject that factory instead of directly injecting the message into every object that needed to deal with emails. I used the factory to overcome the problem posed from the fact that when using DI you generally only have the container manage one instance of a dependency for the whole app lifetime--you don't want to be dealing with a persistent MyEmailTemplate instance. IMyEmailTemplate was an interface I defined to make it testable--it only defines the TransformText() method so I could mock the template output in my tests. I then applied the interface to the template by taking advantage of the fact that the template class that is generated for you is partial.

I had a lot of success with this method, and I think it will work great for you as well.

Addendum: I should add that the rest of the mail functionality can and should be handled with the factory approach. Instead of injecting a MailAddress or MailMessage, which can quickly become problematic, use the factory approach. Additionally, depending on your DI framework, you might be able to directly inject values from your config file into your constructors (I know Windsor supports this).