0
\$\begingroup\$

This function is used in a ASP.NET web application written in C#. It is heavily used to output summary information of the some prices chosen by a user. We are trying to lessen how much we have to scale horizontally.

Here is the original function:

    private static Dictionary<int, string> Cache = new Dictionary<int, string> {{ 1, "Adult" }, { 2, "Child" }};

    public string Build(List<int> priceTypeIds)
    {
        if (session == null)
        {
            return "";
        }

        var uniquePriceTypeIds = priceTypeIds.Distinct();

        var result = new List<string>();

        foreach (var priceTypeId in uniquePriceTypeIds)
        {
            var count = priceTypeIds.Count(x => x == priceTypeId);

            var description = Cache[priceTypeId];

            result.Add($"{count} {description}");
        }

        return string.Join(", ", result);
    }

I rewrote the function to eliminate the amount of string concatenations. But

private static Dictionary<int, string> Cache = new Dictionary<int, string> {{ 1, "Adult" }, { 2, "Child" }};

public static string CreateSummary()
{
    var sb = new StringBuilder();

    List<int> sample = new List<int> { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };

    foreach (var priceTypeId in sample.Distinct())
    {
        if (sb.Length > 0)
        {
            sb.Append(',');
            sb.Append(' ');
        }

        sb.Append('(');
        sb.Append(Sample.Count(x => x == priceTypeId));
        sb.Append(')');
        sb.Append(' ');
        sb.Append(Cache[priceTypeId.ToString()]);
    }

    return sb.ToString();
}

The sample data in the second function is a common use case as we have classes get purchased with 20-30 kids in a single transaction along with chaperones.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ What is Cache? \$\endgroup\$
    – user33306
    Mar 16, 2018 at 4:16
  • 5
    \$\begingroup\$ What task does this method accomplish? Please tell us — see How to Ask. \$\endgroup\$ Mar 16, 2018 at 6:04
  • \$\begingroup\$ Doing sb.Append(', '); instead of sb.Append(','); sb.Append(' '); would save you a method call per iteration. \$\endgroup\$ Mar 16, 2018 at 10:14
  • \$\begingroup\$ What is the context of the method? Is this a stub? This is important because here you are not returning a string and merely setting a local variable, which might cause the CLR to do the most effective optimization, which is to do nothing. How big can the number of distinct values in the list be? Is there a reason to use a List over a Set? \$\endgroup\$ Mar 16, 2018 at 10:16
  • \$\begingroup\$ Thanks @200_success for the reference link! I have updated the question to try and be more inteligible. \$\endgroup\$
    – Sam
    Mar 16, 2018 at 15:52

2 Answers 2

1
\$\begingroup\$

As I see it, your code works fine as it is. You could though improve it a little bit by appending each priceTypeId in one appendFormat operation as in:

string CreateSummary<T>(IEnumerable<T> sample, Dictionary<string, int> cache)
{
  var sb = new StringBuilder();

  foreach (var priceTypeId in sample.Distinct())
  {
    sb.AppendFormat("({0}) {1}, ", sample.Count(x => x.Equals(priceTypeId)), cache[priceTypeId.ToString()]);
  }

  sb.Length -= 2;

  return sb.ToString();
}

As it shows, I have made the function generic (and guessed that cache is a Dictionary?). The last irritating comma (',') is removed by sb.Length -= 2;.


A further improvement could be to use grouping instead of Distinct():

string CreateSummary<T>(IEnumerable<T> sample, Dictionary<string, int> cache)
{
  var sb = new StringBuilder();

  foreach (var group in sample.GroupBy(s => s))
  {
    sb.AppendFormat("({0}) {1}, ", group.Count(), cache[group.Key.ToString()]);
  }

  sb.Length -= 2;

  return sb.ToString();
}

To avoid the overhead of cutting of the length you could use an enumerator instead like this in order to handle the first group separately:

string CreateSummary<T>(IEnumerable<T> sample, Dictionary<string, int> cache)
{
  var sb = new StringBuilder();

  var groups = sample.GroupBy(s => s);
  var enumer = groups.GetEnumerator();
  if (enumer.MoveNext())
  {
    sb.AppendFormat("({0}) {1}", enumer.Current.Count(), cache[enumer.Current.Key.ToString()]);
    while (enumer.MoveNext())
    {
      sb.AppendFormat(", ({0}) {1}", enumer.Current.Count(), cache[enumer.Current.Key.ToString()]);
    }
  }

  return sb.ToString();
}

My own solution would be similar to tinstaafls:

string CreateSummary<T>(IEnumerable<T> sample, Dictionary<string, int> cache)
{
  return string.Join(", ", sample.GroupBy(s => s).Select(gr => $"({gr.Count()}) {cache[gr.Key.ToString()]}"));
}
\$\endgroup\$
3
\$\begingroup\$

It looks to me, that your problem could do with a bit more LINQ and the string concatenation operator. Specifically the group clause. Here's one way it could be done:

public static string Method2()
{
    List<int> sample = new List<int> { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
    1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
    2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2 };

    var data = String.Join(", ", (from int i in sample
                                 group i by i into g
                                 select $"({g.Count()}) {Cache[g.Key.ToString()]}"));

    return data;
}

The efficiency, I think would be better than what you have so far,the Distinct and each Count are extra iterations over the data, but, it would have to be tested to be sure.

It definitely has the advantage of being more concise.

Having the string format in one place makes it easier to see what's happening.

If you need the data parsed, instead of one long string, at some point, removing the String.Join part will leave an IEnumerable<string>.

\$\endgroup\$

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.