Factoring out common elements of an email transmission routine

Question

I wrote some code and realized they share similar logic. I am struggling to share the logic and welcome any suggestions.

public List<string> SendTemplateAsSampleEmail(MtaConnection mtaConnection, ICollection<EmailRecipient> recipients,
           SimpleContentDto renderedContent)
        {
            var errorStrings = new List<string>();

            // Get the list of branded values
            var fromAddress = BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase);
            var replyToAddress = BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase);
            var fromName = BrandingHelper.GetBrandName(_httpContextBase);

            // First send as in TGE
            if (!string.IsNullOrEmpty(renderedContent.HtmlText))
            {
                errorStrings.AddRange(SendTransactionalEmail(mtaConnection, recipients, renderedContent.HtmlText,
                    HtmlSubjectPrefixAsTge + renderedContent.Subject,
                    fromAddress, fromName, replyToAddress,
                    emailDomain: "[email protected]",
                    preHeader: null,
                    contentType: EmailContentType.HtmlContent));
            }
            return errorStrings;
        }
        public List<string> SendSampleEmailForHtmlContent(MtaConnection mtaConnection, ICollection<EmailRecipient> recipients, string name,
            string htmlContent, string preHeader)
        {
            return SendTransactionalEmail(mtaConnection, recipients, htmlContent, HtmlSubjectPrefix + name,
                fromAddress: BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase),
                fromName: BrandingHelper.GetBrandName(_httpContextBase),
                replyToAddress: BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase),
                emailDomain: "[email protected]", preHeader: preHeader,
                contentType: EmailContentType.HtmlContent);
        }

Answer 1

There are many options. The most basic is to write a common routine with two frontends:

public List<string> SendTemplateAsSampleEmail(
    MtaConnection mtaConnection,
    ICollection<EmailRecipient> recipients,
    SimpleContentDto renderedContent)
{
    if (string.IsNullOrEmpty(renderedContent.HtmlText))
        return new List<string>();
    // First send as in TGE
    return SendEmail(
        mtaConnection, recipients, 
        content: renderedContent.HtmlText,
        subjectPrefix: HtmlSubjectPrefixAsTge,
        subject: renderedContent.Subject,
        preHeader: null
    );
}

public List<string> SendSampleEmailForHtmlContent(
    MtaConnection mtaConnection, 
    ICollection<EmailRecipient> recipients, 
    string name,
    string htmlContent, 
    string preHeader)
{
    return SendEmail(
        mtaConnection, recipients, 
        content: htmlContent, 
        subjectPrefix: HtmlSubjectPrefix,
        subject: name,
        preHeader: preHeader,
    );
}

public List<string> SendEmail(
    MtaConnection mtaConnection,
    ICollection<EmailRecipient> recipients,
    string content,
    string subjectPrefix,
    string subject,
    string preHeader)
{
    return SendTransactionalEmail(
        mtaConnection, recipients, content, 
        subjectPrefix + subject,
        fromAddress: BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase),
        fromName: BrandingHelper.GetBrandName(_httpContextBase),
        replyToAddress: BrandingHelper.GetFromAddressForSampleEmail(_httpContextBase),
        emailDomain: "[email protected]", 
        preHeader: preHeader,
        contentType: EmailContentType.HtmlContent
    );
}

But there are other problems with this code. If you ask to send an email with no body, it doesn't seem like a good idea to return a list of no errors - indicating that all went well; instead throw an exception.

Answer 2

Let's talk a little bit about the SendTemplateAsSampleEmail method:

• Perform the preliminary checks first
  • Your method might receive some "bad" input (like empty HtmlText) which can prevent the method to perform any kind of useful thing
  • There is a common pattern called fail fast (or sometimes referred as early exit) which can be useful here

Before

public List<string> SendTemplateAsSampleEmail(...)
{
    var errorStrings = new List<string>();
    var fromAddress = ...;
    var replyToAddress = ...;
    var fromName = ...;

    if (!string.IsNullOrEmpty(renderedContent.HtmlText))
    {
        errorStrings.AddRange(...);
    }
    return errorStrings;
}

After

public List<string> SendTemplateAsSampleEmail(...)
{
    if (string.IsNullOrEmpty(renderedContent.HtmlText))
    {
        return new List<string>();
    }

    var errorStrings = new List<string>();
    var fromAddress = ...;
    var replyToAddress = ...;
    var fromName = ...;

    errorStrings.AddRange(...);
    return errorStrings;
}

• So, instead of surrounding your core logic with a guard expression, you can simply shortcut the execution if some conditions are not met
  • With this approach, you can omit the BrandingHelper calls if the HtmlText is empty
• As it was said by Reinderien it might be a better idea to throw some exception whenever the HtmlText is empty rather than returning an empty collection
• You can further reduce the code by getting rid of the errorStrings variable

public List<string> SendTemplateAsSampleEmail(...)
{
    //Preliminary checks
    if (string.IsNullOrEmpty(renderedContent.HtmlText))
        return new List<string>();

    //Data gathering
    var fromAddress = ...;
    var replyToAddress = ...;
    var fromName = ...;

    //Perform action and transform response to the desired format
    return SendTransactionalEmail(...).ToList();
}

• Because AddRange (Ref) anticipates an IEnumerable<T> that's why we need to call the ToList.
  • Of course if SendTransactionalEmail returns with a List<string> then the ToList can be omitted

---

For the sake of completeness I have to mention that early exit pattern is against the single return law.

---

And one last remark

• It might be advisable to avoid relying on HttpContext in this method
  • Retrieve the necessary data as close to your http layer as possible
  • This will give you reusability, you can call this method for those places as well where the HttpContext might not be available
  • This will also give you loose coupling from the presentation layer