3
\$\begingroup\$

I am currently updating some existing functionality at my work place. This functionality pulls a template of text from a database based on an enum of EmailType. It then replaces placeholders that are within the template with real-time data, mostly Account specific data. The solution itself was written a few years ago and I am looking for a few suggestions on the best way to make it more generic.

As I cannot share my works code, I have written an example to show what I currently have.

Enum is (Real Enum has over 20 types):

public enum EmailType
{
    Type0 = 0,
    Type1 = 1,
    Type2 = 2,
    Type3 = 3,
    Type4 = 4
}

We have a class that manages the placeholders and replaces them with the relevant data. Currently when replacing it will take a PropertiesContainer object, in which the user can set a number of properties, none of which are required, and replaces what it needs to:

public static class Placeholders
{
    public const string CONST_A = "<<ATEST>>";
    public const string CONST_B = "<<BTEST>>";
    public const string CONST_C = "<<CTEST>>";
    public const string CONST_D = "<<DTEST>>";
    public const string CONST_E = "<<ETEST>>";
    public const string CONST_F = "<<FTEST>>";
    public const string CONST_G = "<<GTEST>>";
    public const string CONST_H = "<<HTEST>>";

    public static List<string> GetPlaceholders(EmailType type)
    {
        List<string> placeholders = new List<string>();

        placeholders.Add(CONST_A);
        placeholders.Add(CONST_B);

        switch(type)
        {
            case EmailType.Type0:
                placeholders.Add(CONST_C);
                placeholders.Add(CONST_G);
                break;
            case EmailType.Type1:
                placeholders.Add(CONST_D);
                placeholders.Add(CONST_E);
                break;
            case EmailType.Type2:
                placeholders.Add(CONST_E);
                placeholders.Add(CONST_F);
                break;
            case EmailType.Type3:
                placeholders.Add(CONST_F);
                placeholders.Add(CONST_C);
                break;
            case EmailType.Type4:
                placeholders.Add(CONST_F);
                placeholders.Add(CONST_D);
                break;
        }

        return placeholders;
    }

    public static string ReplacePlaceholders(string content, PropertiesContainer properties)
    {
        string newContent = content;

        if (!string.IsNullOrEmpty(properties.PropertyA))
            newContent.Replace(CONST_A, properties.PropertyA);

        if (!string.IsNullOrEmpty(properties.PropertyB))
            newContent.Replace(CONST_B, properties.PropertyB);

        if (properties.PropertyC != null)
            newContent.Replace(CONST_C, properties.PropertyC.ToString());

        if (properties.PropertyD != null)
            newContent.Replace(CONST_D, properties.PropertyD.ToString());

        if (properties.PropertyE != null)
            newContent.Replace(CONST_E, properties.PropertyE.ToString());

        if (properties.PropertyF != null)
            newContent.Replace(CONST_F, properties.PropertyF.ToString());

        if (properties.PropertyG != null)
            newContent.Replace(CONST_G, properties.PropertyG.ToString());

        if (properties.PropertyH != null)
            newContent.Replace(CONST_H, properties.PropertyH.ToString());

        return newContent;
    }

    public static string GetTemplate(EmailType type)
    {
        switch (type)
        {
            case EmailType.Type0:
                return "User <<ATEST>> has a balance of <<GTEST>>";
            case EmailType.Type1:
                return "User <<ATEST>> was added on <<ETEST>>";
            case EmailType.Type2:
                return "<<ETEST>> - <<FTEST>>";
            case EmailType.Type3:
                return "<<FTEST>> - <<CTEST>>";
            case EmailType.Type4:
                return "<<FTEST>> - <<DTEST>>";
        }
        return string.Empty;
    }

And then we have a class which will get the relevant template, call Placeholders to replace the relevant data, and then send the email.

public void SendEmailA(Account account)
    {
        // Get the Email template
        string template = Placeholders.GetTemplate(EmailType.Type0);

        PropertiesContainer properties = new PropertiesContainer {
            PropertyA = account.AccountName,
            PropertyG = account.Balance
        };

        string body = Placeholders.ReplacePlaceholders(template, properties);

        SendEmail(body, "A Subject", "[email protected]", "[email protected]");
    }

    public void SendEmailB(Account account)
    {
        // Get the Email template
        string template = Placeholders.GetTemplate(EmailType.Type1);

        PropertiesContainer properties = new PropertiesContainer
        {
            PropertyD = account.AccountID,
            PropertyE = account.Created
        };

        string body = Placeholders.ReplacePlaceholders(template, properties);

        SendEmail(body, "A Subject", "[email protected]", "[email protected]");
    }

    public void SendEmailC(Account account)
    {
        // Get the Email template
        string template = Placeholders.GetTemplate(EmailType.Type2);

        PropertiesContainer properties = new PropertiesContainer
        {
            PropertyE = account.Created,
            PropertyF = account.Left
        };

        string body = Placeholders.ReplacePlaceholders(template, properties);

        SendEmail(body, "A Subject", "[email protected]", "[email protected]");
    }

    public void SendEmailD(Account account)
    {
        // Get the Email template
        string template = Placeholders.GetTemplate(EmailType.Type3);

        PropertiesContainer properties = new PropertiesContainer
        {
            PropertyF = account.Created,
            PropertyC = account.AccountID
        };

        string body = Placeholders.ReplacePlaceholders(template, properties);

        SendEmail(body, "A Subject", "[email protected]", "[email protected]");
    }

    public void SendEmailE(Account account)
    {
        // Get the Email template
        string template = Placeholders.GetTemplate(EmailType.Type4);

        PropertiesContainer properties = new PropertiesContainer
        {
            PropertyF = account.Created,
            PropertyD = account.AccountID
        };

        string body = Placeholders.ReplacePlaceholders(template, properties);

        SendEmail(body, "A Subject", "[email protected]", "[email protected]");
    }

The way that it is written, where you can populate the PropertiesContainer with any relevant data, and then when calling Placeholders.ReplacePlaceholders() it does not seem very efficient to have a long list of IF statements determining if we should replace the text or not.

Any suggestions on how this code could be made more efficient and nicer to understand would be great.

\$\endgroup\$
3
  • \$\begingroup\$ This looks like a typical academic or book example, completely incomprehensible, full of meaningless generic names. \$\endgroup\$
    – t3chb0t
    Commented Jun 14, 2017 at 15:45
  • 1
    \$\begingroup\$ Haha don't be so harsh @t3chb0t we are all learning :) \$\endgroup\$
    – Denis
    Commented Jun 14, 2017 at 15:46
  • \$\begingroup\$ As I stated in the post, this is not the actual code in use, it is just an example of the code architecture that is in use, hence the generic names. \$\endgroup\$ Commented Jun 15, 2017 at 11:59

2 Answers 2

3
\$\begingroup\$

Overview

Your code doesn't look good. Here are some of the reasons why.

  • You have a bug
  • You have tons of repetitive code
  • Your naming is poor e.g PropertyA, PropertyB...
  • It would be difficult to extend and modify with all of those switch-cases
  • Magic!

    Take a look at this:

     case EmailType.Type0:
        placeholders.Add(CONST_C);
        placeholders.Add(CONST_G);
    

    I don't have any idea what EmailType.Type0 means and it's not apparent what the difference is between the different email types. Additionally, I don't know how EmailType.Type0 relates to the const values C and G.

There some other minor things such as using the add method instead of collection initializers, unnecessary comments, however I won't touch on those because you should first understand why and how to avoid your major problems.

I believe there are major design problems with your code but I can't comment on them either as there is not sufficient information about the other classes.

Bug

I don't know if you've tested your program but this chunk of code won't do what you expect:

if (!string.IsNullOrEmpty(properties.PropertyA))
    newContent.Replace(CONST_A, properties.PropertyA);

if (!string.IsNullOrEmpty(properties.PropertyB))
    newContent.Replace(CONST_B, properties.PropertyB);
//...

Specifically the lines where you replace the string. This method doesn't modify the input. Strings are a bit complicated when it comes to their behavior.

Here are some of the characteristics of the string object:

  1. This is a reference type object.

  2. This is an immutable object.

Now these 2 are fighting in a way, because of the immutability that the string has it won't behave like a reference type but rather like a value type object. When you pass a string as a parameter to a method, changes within the function are not visible outside of the function unless you consume the object that is returned.

The point of the matter is that you should do the following instead:

newContent = newContent.Replace(CONST_A, properties.PropertyA);

Reducing the repetitive code

I'm look at all of your SendEmailX methods

Let's take a look at 2 of your SendEmail functions:

public void SendEmailA(Account account)
{
    // Get the Email template
    string template = Placeholders.GetTemplate(EmailType.Type0);

    PropertiesContainer properties = new PropertiesContainer
    {
        PropertyA = account.AccountName,
        PropertyG = account.Balance
    };

    string body = Placeholders.ReplacePlaceholders(template, properties);

    SendEmail(body, "A Subject", "[email protected]", "[email protected]");
}

public void SendEmailB(Account account)
{
    // Get the Email template
    string template = Placeholders.GetTemplate(EmailType.Type1);

    PropertiesContainer properties = new PropertiesContainer
    {
        PropertyD = account.AccountID,
        PropertyE = account.Created
    };

    string body = Placeholders.ReplacePlaceholders(template, properties);

    SendEmail(body, "A Subject", "[email protected]", "[email protected]");
}

First off let's start with the obvious you can see that the last 2 lines of your method body are always the same, there is absolutely no difference, so it shouldn't be a problem extracting that right away.

One way to tackle the rest of the code that depends on some variable is to create a dictionary which maps the email types to some action that you're going to execute on each type:

private static readonly Dictionary<EmailType, Func<EmailType, Account, PropertiesContainer>>
    emailTypeToPropertiesContainer = new Dictionary<EmailType, Func<EmailType, Account, PropertiesContainer>>
    {
        [EmailType.Type0] = (type, account) => new PropertiesContainer
        {
            PropertyA = account.AccountName,
            PropertyG = account.Balance
        },
        [EmailType.Type1] = (type, account) => new PropertiesContainer
        {
            PropertyD = account.AccountID,
            PropertyE = account.Created
        },
        [EmailType.Type2] = (type, account) => new PropertiesContainer
        {
            PropertyE = account.Created,
            PropertyF = account.Left
        },
        [EmailType.Type3] = (type, account) => new PropertiesContainer
        {
            PropertyF = account.Created,
            PropertyC = account.AccountID
        },
        [EmailType.Type4] = (type, account) => new PropertiesContainer
        {
            PropertyF = account.Created,
            PropertyD = account.AccountID
        },
    };

With that your method can look like this:

public void SendEmailX(Account account, EmailType emailType)
{
    string template = Placeholders.GetTemplate(emailType);
    PropertiesContainer properties = emailTypeToPropertiesContainer[emailType].Invoke(emailType, account);
    string body = Placeholders.ReplacePlaceholders(template, properties);

    SendEmail(body, "A Subject", "[email protected]", "[email protected]");
}

You apply the same technique to your GetPlaceholders function:

private static readonly Dictionary<EmailType, IEnumerable<string>> emailTypeValues =
    new Dictionary<EmailType, IEnumerable<string>>
    {
        [EmailType.Type0] = new[] {CONST_C, CONST_G},
        [EmailType.Type1] = new[] {CONST_D, CONST_E},
        [EmailType.Type2] = new[] {CONST_E, CONST_F},
        [EmailType.Type3] = new[] {CONST_F, CONST_C},
        [EmailType.Type4] = new[] {CONST_F, CONST_D},
    };

public static IEnumerable<string> GetPlaceholders(EmailType type)
{
    return emailTypeValues[type];
}

I will leave up to you to create something similar GetTemplate method. You might even consider making additional objects to ease the initialization of those dictionaries or even remove the need for them.

\$\endgroup\$
0
\$\begingroup\$

Your current code is not very easy to maintain and understand because business logic is spread between different types. Consider what you should to do when new type of email is added:

  1. Create a new member in EmailType enum
  2. Add new template in Placeholders.GetTemplate method which returns new template string for new enum member
  3. Add required relevant placeholders to Placeholders.GetPlaceholders method to return placeholders which are used by the template on the previous step. I feel uncomfortable here, because this is a definitely duplication of knowledge - we already have specified placeholders when defined template on step 2. Also we can forget to add some placeholder which is used in template.
  4. Create new SendEmailX method which will duplicate knowledge again - it should know which properties should be set in PropertiesContainer. And those properties should match placeholders from step 3 which in turn should match template from step 2.

In the perfect world, I would like to keep all this knowledge in one place. E.g. you can use the power of C# interpolated strings which is a template string with interpolated expressions:

public void SendEmailB(Account account)
{
    string body = $"User {account.AccountID} was added on {account.Created}";
    SendEmail(body, "Subject B", "[email protected]", "[email protected]");
}

Here you don't have any duplication of knowledge - you specify email body template, placeholders and provide values to placeholders without setting some properties of PropertiesContainer and passing it through the system (keeping in mind that PropertyB is mapped to CONST_B and should have a value of AccountID).


Even more, if all your email messages use account attributes, then you can create set of account extension methods:

public static string ToBalanceMessage(this Account account)
  => $"User {account.AccountName} has a balance of {account.Balance}";

And sending email will look like (SendEmailA, SendEmailB etc are not needed anymore):

SendEmail(account.ToBalanceMessage(), "Subject A", "[email protected]", "[email protected]");
\$\endgroup\$
2
  • \$\begingroup\$ Ik think the extension method approach is a bit to generic, for a specific task. Rather just use SomeClass.CreateBalanceMessage(Account account) \$\endgroup\$
    – Myrtle
    Commented Jun 15, 2017 at 11:45
  • \$\begingroup\$ @Aphelion that's why extensions method approach is an optional solution to think about. Those extensions should be defined in the separate namespace and used only at email sending class. I don't think about extension methods as a solution which should be used globally. It's just a way to make code more readable and fluent. I often use local extension methods. Especially when some data transformation is required: date.ToShortDateString(), entity.ToModel(), account.ToBallanceMessage() \$\endgroup\$ Commented Jun 15, 2017 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.