Conditional string building

Question

Essentially I'm taking a number of checkboxes off of my view, adding them to a Dictionary<string, bool>, and writing out the Keys of any checkbox that is checked.

I'm guessing there must be a better way of doing this.

// We need to build a list of what the user would like more information on.

// First step is to add all of the Checkboxes to a `Dictionary<string, bool>`
Dictionary<string,bool> moreInfo = new Dictionary<string,bool>();
moreInfo.Add("Children's Programs", ccm.moreInfoChildren);
moreInfo.Add("Youth Programs",ccm.moreInfoYouth);
moreInfo.Add("Small Groups", ccm.moreInfoSmallGroups);
moreInfo.Add("Getting involved in Ministry",ccm.moreInfoMinistry);
moreInfo.Add("Receiving offering envelopes",ccm.moreInfoOfferingEnvelopes);
moreInfo.Add("Church Membership",ccm.moreInfoMembership);
moreInfo.Add("Baptism",ccm.moreInfoBaptism);
moreInfo.Add("Other (see notes)", ccm.moreInfoOther);


// Second step is to loop through the moreInfo `Dictionary<string, bool>` and build a string of results.
StringBuilder _moreInfo = new StringBuilder();
foreach (var info in moreInfo) {
    _moreInfo.Append(info.Value ? info.Key + ", " : "");
}

// Third step is to determin if the user wants any information. If they do, we add that information to the email.
if(!string.IsNullOrEmpty(_moreInfo.ToString()))
{
    builder.Append("I would like more information about ");
    builder.Append(_moreInfo.ToString());
}

Here is the complete Action:

[HttpGet]
[AllowCrossSiteJsonAttribute]
public JsonpResult CommunicateCard(CommunicateCardModel ccm)
{
    if (ModelState.IsValid)
    {
        StringBuilder builder = new StringBuilder();

        // ccm.name is a required field, so we don't have to check if it's empty.
        builder.Append("Name: " + ccm.name + "\n");

        // If these properties are empty, we don't want to include them in the built string.
        builder.Append((!string.IsNullOrEmpty(ccm.address)) ? "Address: " + ccm.address + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.city)) ? "City: " + ccm.city + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.postalCode)) ? "Postal Code: " + ccm.postalCode + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.phone)) ? "Phone Number: " + ccm.phone + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.emailFrom)) ? "Email Address: " + ccm.emailFrom + "\n" : null);

        builder.Append("\n");

        builder.Append((!string.IsNullOrEmpty(ccm.information)) ? "Please " + ccm.information + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.christianity)) ? "I " + ccm.christianity + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.maritalStatus)) ? "I am " + ccm.maritalStatus + "\n" : null);

        builder.Append("\n");


        // pull together all of the checkboxes within the "Ministries" fieldset and generate a single string.
        var pairs = new[]
        {
            // Tuple.Create(string, bool)
            Tuple.Create("Children's Programs", ccm.moreInfoChildren),
            Tuple.Create("Youth Programs", ccm.moreInfoYouth),
            Tuple.Create("Small Groups", ccm.moreInfoSmallGroups),
            Tuple.Create("Getting involved in Ministry", ccm.moreInfoMinistry),
            Tuple.Create("Receiving offering envelopes", ccm.moreInfoOfferingEnvelopes),
            Tuple.Create("Church Membership", ccm.moreInfoMembership),
            Tuple.Create("Baptism", ccm.moreInfoBaptism),
            Tuple.Create("Other (see notes)", ccm.moreInfoOther),
        };

        var moreInfo = String.Join(", ", pairs.Where(pair => pair.Item2).Select(pair => pair.Item1));

        if (!String.IsNullOrEmpty(moreInfo))
        {
            builder.Append("I would like more information about ")
                   .Append(moreInfo);
        }

        builder.Append("\n");

        builder.Append((!string.IsNullOrEmpty(ccm.guestReason)) ? "I am visiting today because " + ccm.guestReason + "\n" : null);
        builder.Append((!string.IsNullOrEmpty(ccm.attendance)) ? "I am attending for my " + ccm.attendance + "\n" : null);

        builder.Append("\n");

        builder.Append((!string.IsNullOrEmpty(ccm.comments)) ?  ccm.comments : null);


        // initiate the email service and fire off an email
        EmailService email = new EmailService();
        email.to = ccm.emailTo;
        email.from = ccm.emailTo;
        email.replyTo = ccm.emailFrom;
        email.subject = ccm.messageSubject;
        email.messageBody = builder.ToString();

        bool sent = email.send();
        object result = null;

        // Send a system error message via JSON back to the app if the email fails.
        if (!sent)
        {
            result = new ErrorsModel { ErrorMessage = Server.GetLastError().Message };
        }

        // whether or not the email sent, we need to tell the app the result.
        return this.Jsonp(result, sent);
    }
    // The ModelState is invalid, Loop through the ModelState and send a JSON string listing all the errors.
    else 
    {
        List<ErrorsModel> Errors = new List<ErrorsModel>();
        foreach(ModelState modelState in ModelState.Values)
        {
            foreach (ModelError error in modelState.Errors)
            {
                Errors.Add(new ErrorsModel { ErrorMessage = error.ErrorMessage });
            };
        };

        return this.Jsonp(Errors, false);
    }
}

Answer 1

Your method has a lot of repetition to build your string often resulting in unnecessary calls to Append(). You should have used an if block for each of those writes instead of appending null. This also hides all the calls to IsNullOrEmpty() which is a detail you don't really want repeated so much. I'd break this into a couple of methods, one for each of the valid/invalid cases and one to help with writing the strings. Written this way, you have the option to use the same trick to quickly build the comma-separated list to write your content easier to do. I think you'll agree that it's a lot more easier to read.

[HttpGet]
[AllowCrossSiteJsonAttribute]
public JsonpResult CommunicateCard(CommunicateCardModel ccm)
{
    return ModelState.IsValid
        ? CommunicateValidCard(ccm)
        : CommunicateInvalidCard(ccm);
}

StringBuilder AppendNonWhiteField(StringBuilder builder, string format, string field)
{
    if (!String.IsNullOrWhiteSpace(field))
    {
        builder.AppendFormat(format, field);
    }
    return builder;
}

JsonpResult CommunicateValidCard(CommunicateCardModel ccm)
{
    StringBuilder builder = new StringBuilder();

    // ccm.name is a required field, so we don't have to check if it's empty.
    builder.Append("Name: " + ccm.name + "\n");

    // If these properties are empty, we don't want to include them in the built string.
    AppendNonWhiteField(builder, "Address: {0}\n",       ccm.address);
    AppendNonWhiteField(builder, "City: {0}\n",          ccm.city);
    AppendNonWhiteField(builder, "Postal Code: {0}\n",   ccm.postalCode);
    AppendNonWhiteField(builder, "Phone Number: {0}\n",  ccm.phone);
    AppendNonWhiteField(builder, "Email Address: {0}\n", ccm.emailFrom);
    builder.AppendLine();

    AppendNonWhiteField(builder, "Please {0}\n", ccm.information);
    AppendNonWhiteField(builder, "I {0}\n",      ccm.christianity);
    AppendNonWhiteField(builder, "I am {0}\n",   ccm.maritalStatus);
    builder.AppendLine();

    // pull together all of the checkboxes within the "Ministries" fieldset and generate a single string.
    var pairs = new[]
    {
        // Tuple.Create(string, bool)
        Tuple.Create("Children's Programs",          ccm.moreInfoChildren),
        Tuple.Create("Youth Programs",               ccm.moreInfoYouth),
        Tuple.Create("Small Groups",                 ccm.moreInfoSmallGroups),
        Tuple.Create("Getting involved in Ministry", ccm.moreInfoMinistry),
        Tuple.Create("Receiving offering envelopes", ccm.moreInfoOfferingEnvelopes),
        Tuple.Create("Church Membership",            ccm.moreInfoMembership),
        Tuple.Create("Baptism",                      ccm.moreInfoBaptism),
        Tuple.Create("Other (see notes)",            ccm.moreInfoOther),
    };
    var moreInfo = String.Join(", ", pairs.Where(pair => pair.Item2).Select(pair => pair.Item1));
    AppendNonWhiteField(builder, "I would like more information about {0}\n", moreInfo);

    AppendNonWhiteField(builder, "I am visiting today because {0}\n", ccm.guestReason);
    AppendNonWhiteField(builder, "I am attending for my {0}\n",       ccm.attendance);
    builder.AppendLine();

    AppendNonWhiteField(builder, "{0}", ccm.comments);

    // initiate the email service and fire off an email
    EmailService email = new EmailService();
    email.to = ccm.emailTo;
    email.from = ccm.emailTo;
    email.replyTo = ccm.emailFrom;
    email.subject = ccm.messageSubject;
    email.messageBody = builder.ToString();

    bool sent = email.send();
    object result = null;

    // Send a system error message via JSON back to the app if the email fails.
    if (!sent)
    {
        result = new ErrorsModel { ErrorMessage = Server.GetLastError().Message };
    }

    // whether or not the email sent, we need to tell the app the result.
    return this.Jsonp(result, sent);
}

JsonpResult CommunicateInvalidCard(CommunicateCardModel ccm)
{
    // The ModelState is invalid, Loop through the ModelState and send a JSON string listing all the errors.
    var errors = ModelState.Values
        .SelectMany(modelState =>
            modelState.Errors
                .Select(error => new ErrorsModel { ErrorMessage = error.ErrorMessage }))
        .ToList();
    return this.Jsonp(errors, false);
}

Answer 2

Without knowing if you need the dictionary for later use, you can absolutely make the 2nd step better (and remove the trailing ", " that would get added) by using String.Join with a lambda (don't forget to add the System.Linq using if it is not there):

// Second step is to loop through the moreInfo `Dictionary<string, bool>` and build a string of results.
string _moreInfo = String.Join(", ", moreInfo.Where(m => m.Value).Select(m => m.Key).ToArray());

Then you can also drop the .ToString() on the IsNullOrEmpty check of _moreInfo and the append of _moreInfo in step 3.