0
\$\begingroup\$

How is this function?

/// <summary>Extension methods used by RedViewerLibrary.</summary>
internal static class Extensions
{
    /// <summary>A string array extension method that gets the human readable enumerated version of the array.</summary>
    /// <param name="items">The strings to act on.</param>
    /// <returns>A string containing the enumerated results.</returns>
    /// <example>null => ""</example>
    /// <example>{} => ""</example>
    /// <example>{"foo"} => "foo"</example>
    /// <example>{"foo", "bar"} => "foo, and bar"</example>
    /// <example>{"foo", "bar", "baz"} => "foo, bar, and baz"</example>
    internal static string GetEnumerated(this string[] items)
    {
        if (items == null || items.Length == 0)
        {
            return string.Empty;
        }
        else if (items.Length == 1)
        {
            return items[0];
        }

        const string COMMA_SPACE = ", ";
        const string AND_STRING = "and ";

        var totalNamesLength = items.Aggregate(0, (oldValue, name) => name.Length + oldValue);
        var numberOfCommaSpaces = items.Length - 1;
        var commaSpacesLength = numberOfCommaSpaces * COMMA_SPACE.Length;
        var predictedLength = totalNamesLength + commaSpacesLength + AND_STRING.Length;
        var sb = new StringBuilder(predictedLength);

        int loopCount = items.Length - 1;
        sb.Append(items[0]);
        for (int idx = 1; idx != loopCount; ++idx)
        {
            sb.Append(COMMA_SPACE);
            sb.Append(items[idx]);
        }

        sb.Append(COMMA_SPACE);
        sb.Append(AND_STRING);
        sb.Append(items[items.Length - 1]);
        return sb.ToString();
    }
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Does this "and" make it human readable? A lot of code for what could be just String.Join(", ", array). \$\endgroup\$ Aug 30, 2012 at 11:57

2 Answers 2

8
\$\begingroup\$

It's ok. A bit hard to follow however since you're taking steps that really aren't necessary.

  • It is not necessary to give the StringBuilder a starting capacity. Just let it do its business and you do yours, it will just work.

    However if you really wanted to do this, then that's not a problem. You can clean it up a bit. Rather than aggregating over the items when adding up all the lengths, just use Sum() instead.

    var totalNamesLength = items.Sum(s => s.Length);
    
  • Don't really have much of a comment on the rest of your code. I'd be careful of using ==/!= comparisons in a loop that goes over consecutive values, if the loop variable were to ever change in the body of the loop, you'll have a hell of a time trying to debug any problems you have with that. I'd stick to using < (or appropriate operator) there exclusively.

    for (int idx = 1; idx < loopCount; ++idx)
        // ...
    

I'd write this differently however as this is much more readable to me and compact.

internal static string GetEnumerated(this string[] items)
{
    if (items == null)
        return "";

    if (items.Length <= 1)
        // concatenates all the (0 or 1) items into a single string
        return String.Concat(items);

    // place all but the last in a comma-separated string
    var commaSeparated = String.Join(", ", items.Take(items.Length - 1));
    // include the last item
    return commaSeparated + ", and " + items.Last();
}

Since we know we are dealing with arrays, you won't be paying for much in terms of performance in the LINQ calls. If that bothers you, it is simple to write the equivalent without using LINQ.

internal static string GetEnumerated(this string[] items)
{
    // I'd leave these in separate conditions
    // to make it clear they are separate cases
    if (items == null)
        return "";
    if (items.Length <= 1)
        return String.Concat(items);

    var commaSeparated = String.Join(", ", items, 0, items.Length - 1);
    return commaSeparated + ", and " + items[items.Length - 1];
}
\$\endgroup\$
1
\$\begingroup\$

I'd make it a little more applicable than just to arrays (via IEnumerable<string>) and even further, use it with lists of all kinds as long as the ToString() gives a readable version:

/// <summary>Extension methods used by RedViewerLibrary.</summary>
internal static class Extensions
{
    /// <summary>A string array extension method that gets the human readable enumerated version of the array.</summary>
    /// <param name="items">The strings to act on.</param>
    /// <returns>A string containing the enumerated results.</returns>
    /// <example>null => ""</example>
    /// <example>{} => ""</example>
    /// <example>{"foo"} => "foo"</example>
    /// <example>{"foo", "bar"} => "foo, and bar"</example>
    /// <example>{"foo", "bar", "baz"} => "foo, bar, and baz"</example>
    internal static string GetEnumerated<T>(this IEnumerable<T> items)
    {
        if (items == null || !items.Any())
        {
            return string.Empty;
        }

        var count = items.Count();

        if (count == 1)
        {
            return items.First().ToString();
        }

        const string CommaSpace = ", ";
        const string AndString = "and ";
        var sb = new StringBuilder();

        sb.Append(items.First());
        foreach(var item in items.Skip(1).Take(count - 2))
        {
            sb.Append(CommaSpace);
            sb.Append(item);
        }

        sb.Append(CommaSpace);
        sb.Append(AndString);
        sb.Append(items.Last());
        return sb.ToString();
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ The downside of doing this over the array version is that it allocates much more memory and will have much worse constant factors (.Count() is linear time, n virtual dispatches for ToString calls, allocations for enumerators and such). (using IEnumerator<T> directly helps but doesn't completely eliminate these factors) \$\endgroup\$ Aug 5, 2015 at 1:48
  • \$\begingroup\$ Similar to ideone.com/z1l1oy \$\endgroup\$ Aug 5, 2015 at 1:52
  • \$\begingroup\$ .Count() is not necessarily linear time. It does a check to see if the underlying type implements ICollection or ICollection<T> (which arrays and lists do) and simply passes it along from the .Count property. \$\endgroup\$ Aug 5, 2015 at 3:28
  • \$\begingroup\$ Sure, but with a large percentage of IEnumerables, ICollection isn't in use. Even simple things like "foArray.Skip(2)" fail that test. \$\endgroup\$ Aug 5, 2015 at 4:11

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.