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

Make Email Sender more maintainable and testable

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewemailobject-orientedcsharpstackoverflowc#
emailtestablesendermakemaintainablemoreand

Problem

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(

Solution

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.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 dict = new Dictionary {
{ "orderref", order.OrderID.ToString()) }
{ "orderstatus", EnumHelper.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);
bodyPie

Code Snippets

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());
}
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());
}
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());
}

Context

StackExchange Code Review Q#88145, answer score: 3

Revisions (0)

No revisions yet.