8
\$\begingroup\$

I've got a fairly simple task here, but I think it's worth trying to perfect the implementation, because I can probably learn a thing or two about using .NET's enumerables correctly in the process.

I have two "lists" of strings, let's call them values and separators. The former is one element longer than the latter, and I want to interleave them into a single string (so I want to riffle the separators in between the values). Example:

var values = new []{ "abc", "def", "ghi" };
var separators = new []{ "123", "456" };

// expected result: "abc123def456ghi"

I need this in a few places, and in my actual code these might be Lists or other enumerables, so I figured I'd add an extension method to IEnumerable<string>, which I call on the values and pass the separators as an argument. Here's what I've got:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Retina.Extensions
{
    public static class EnumerableExtension
    {
        public static string Riffle(this IEnumerable<string> values, IEnumerable<string> separators)
        {
            if (values.Count() != separators.Count() + 1)
                throw new Exception("Riffled enumerable needs to be one shorter than this enumerable.");

            var builder = new StringBuilder();

            var zipped = values.Zip(separators, (val, sep) => new { val, sep });

            foreach (var pair in zipped)
            {
                builder.Append(pair.val);
                builder.Append(pair.sep);
            }

            builder.Append(values.Last());

            return builder.ToString();
        }
    }
}

As I understand it, IEnumerable is generally a lazy list, so both Count() and Last() potentially incur an additional iteration through the entire list, which seems unnecessary. Also, the intermediate zipped enumerable seems awkward for something as simple as alternatingly iterating through two lists.

Is there a more idiomatic way to implement this and make better use of .NET's collections/enumerables? Is IEnumerable even the correct choice here?

\$\endgroup\$
1
  • \$\begingroup\$ Hint: yield [complying w/ comment min length requirement] \$\endgroup\$
    – radarbob
    Commented Dec 22, 2017 at 7:16

2 Answers 2

6
\$\begingroup\$

You're right about enumerating both parameters multiple times. Each call to Count or Last will execute the enumerator. This might of course become very inefficient, especially if your collections have a lot of items.

So what could you do instead? Well, one option would be to replace IEnumerable with the next closest type which is the ICollection. This would guarantee that both collections are already materialized. This however is rarely a good solution so it's preferable to keep it lazy.

But for this, simple LINQ is not enough. You need to work with enumerators directly and implement your extension in such a way that it uses the GetEnumerator method. With it you have the complete control over the enumeration process. With a little bit of logic you can keep you parameters lazy and still be able to validate that the are more values then separators.

You do this by first getting the first value. Then the while will try to get a pair of separators and values to append them to the string. If at some point it could get a separator but not a value, it means that there is at least the same number of separators as values and at this point you can throw an exception. The other case is when there are not enough separators. To detect this you just need to try to get another value after the while. If it works then it means there were not enough separators.

public static string Join(this IEnumerable<string> values, IEnumerable<string> separators)
{
    var result = new StringBuilder();

    using (var value = values.GetEnumerator())
    using (var separator = separators.GetEnumerator())
    {
        if (value.MoveNext())
        {
            result.Append(value.Current);

            while (separator.MoveNext())
            {
                if (value.MoveNext())
                {
                    result
                        .Append(separator.Current)
                        .Append(value.Current);
                }
                else
                {
                    throw new ArgumentException("There are too many separators!");
                }
            }

            if (value.MoveNext())
            {
                throw new ArgumentException("There are not enough separators!");
            }
        }

        return result.ToString();
    }
}

I would also call this extension Join because this is what it does.

\$\endgroup\$
6
  • \$\begingroup\$ +1 Good one, except for one small detail: this doesn't throw if there are too many values. \$\endgroup\$ Commented Dec 21, 2017 at 20:22
  • \$\begingroup\$ This is nice, thank you! Good catch on chaining the Append commands, and as opposed to my original code this also works when the value list is empty. :) I suppose the way to address Pieter's comment would be to do a if (value.MoveNext()) throw ... after the loop? \$\endgroup\$ Commented Dec 21, 2017 at 20:26
  • \$\begingroup\$ @PieterWitvoet oops, you're right. I've added a fix. \$\endgroup\$
    – t3chb0t
    Commented Dec 21, 2017 at 20:27
  • \$\begingroup\$ @MartinEnder exactly! I guess we wrote it at the same time ;-) \$\endgroup\$
    – t3chb0t
    Commented Dec 21, 2017 at 20:28
  • \$\begingroup\$ Remember the "exit early" principle. Change the outermost if (value.MoveNext()) to if (!value.MoveNext()) return ""; then you don't have to indent the whole code. \$\endgroup\$
    – Snowbody
    Commented Dec 22, 2017 at 7:09
4
\$\begingroup\$

Whether or not Count() requires a new iteration depends on how the IEnumerable<T> object is wired. The framework is smart enough to know that if the object implements ICollection<T> ("unordered") it should invoke the Count property. If not, GetEnumerator() is invoked and the returned enumerator iterated.

The same logic applies for Last(). If the object implements IList<T> (ordered), then the Count property is used to calculate the position, and a call to the indexer returns the element.

So by using your example variables, no extra iteration is required as string[] implements both ICollection<string> and IList<string>.

Now, with that being said, I would rather move the constraint check from the callee to the caller, rending the function more flexible. Then invoke string.Join which under the hood use StringBuilder to compose the final string.

public static class EnumerableExtensions
{

    public static IEnumerable<TSource> Interleave<TSource>(this IEnumerable<TSource> source1, IEnumerable<TSource> source2)
    {

        if (source1 == null) { throw new ArgumentNullException(nameof(source1)); }
        if (source2 == null) { throw new ArgumentNullException(nameof(source2)); }

        using (var enumerator1 = source1.GetEnumerator())
        {
            using (var enumerator2 = source2.GetEnumerator())
            {

                var continue1 = true;
                var continue2 = true;

                do
                {

                    if (continue1 = enumerator1.MoveNext())
                    {
                        yield return enumerator1.Current;
                    }

                    if (continue2 = enumerator2.MoveNext())
                    {
                        yield return enumerator2.Current;
                    }

                }
                while (continue1 || continue2);

            }
        }

    }

}

Example:

https://dotnetfiddle.net/CbSXp2

string[] values;
string[] separators;

values = new[] { "a", "b", "c" };
separators = new[] { "1", "2" };
Console.WriteLine(string.Join("", values.Interleave(separators)));

values =  new[] { "a", "b" };
separators = new[] { "1", "2", "3", "4", "5", "6" }; 
Console.WriteLine(string.Join("", values.Interleave(separators)));

values = new[] { "a", "b", "c", "d", "e", "f" };
separators = new[] { "1", "2" };
Console.WriteLine(string.Join("", values.Interleave(separators)));

a1b2c
a1b23456
a1b2cdef

\$\endgroup\$
2
  • \$\begingroup\$ Unfortunately it is supposed to be an error if the size of values is anything other than one more than the size of separators. \$\endgroup\$
    – Snowbody
    Commented Dec 22, 2017 at 18:44
  • \$\begingroup\$ Understood. But I still think the "best" design would be to leave the constraint check to the caller and not the callee. :) \$\endgroup\$ Commented Dec 22, 2017 at 19:01

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.