3
\$\begingroup\$

I sometimes need to enumerate the same collection multiple times so I wrote an extension that does this for me. I call it Loop. It takes one optional parameter startAt which allows it to start at an offset.


The extension works with enumerators. If it could not MoveNext then it gets a new enumerator and tries again. If the second try failed then the collection is empty, otherwise it starts from the beginning. It uses a local function that handles the enumerator by resetting it if necessary.

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int startAt = 0)
{
    if (values == null)
    {
        throw new ArgumentNullException(nameof(values));
    }

    var moves = 0;

    // ReSharper disable once PossibleMultipleEnumeration
    var enumerator = values.GetEnumerator();

    try
    {
        while (true)
        {
            if (TryMoveNext(enumerator, out enumerator))
            {
                moves++;

                if (startAt > 0 && moves <= startAt)
                {
                    continue;
                }

                yield return enumerator.Current;
            }
            else
            {
                yield break;
            }
        }
    }
    finally
    {
        enumerator.Dispose();
    }

    bool TryMoveNext(IEnumerator<T> currentEnumerator, out IEnumerator<T> newEnumerator)
    {
        if (currentEnumerator.MoveNext())
        {
            newEnumerator = currentEnumerator;
            return true;
        }
        else
        {
            // Get a new enumerator because we took all elements and try again.

            currentEnumerator.Dispose();

            // ReSharper disable once PossibleMultipleEnumeration
            newEnumerator = values.GetEnumerator();

            // If we couldn't move after reset then we're done trying because the collection is empty.
            return newEnumerator.MoveNext();
        }
    }
}

I have the feeling like I might have overcomplicated the implementation.


Here are the tests I wrote for it:

[TestClass]
public class enumerableTest
{
    [TestMethod]
    public void Loop_EmptyCollection_NoneTakes()
    {
        var numbers = new int[0];
        Assert.IsFalse(numbers.Loop().Take(2).Any());
    }

    [TestMethod]
    public void Loop_LessThanAvailable_NoLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2 }, numbers.Loop().Take(2));
    }

    [TestMethod]
    public void Loop_MoreThanAvailable_OneLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2, 3, 1 }, numbers.Loop().Take(4));
    }

    [TestMethod]
    public void Loop_TwiceTheAvailable_TwoLoops()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 1, 2, 3, 1, 2, 3, 1, 2 }, numbers.Loop().Take(8));
    }

    [TestMethod]
    public void Loop_MoreThanAvailableWithStartAt_OneLoop()
    {
        var numbers = new[] { 1, 2, 3 };
        Assert.That.Collection().AreEqual(new[] { 2, 3, 1, 2, 3, 1 }, numbers.Loop(1).Take(6));
    }

}
\$\endgroup\$
4
\$\begingroup\$

Code optimization

This can be shortened:

while (true)
{
    if (TryMoveNext(enumerator, out enumerator))
    {
        moves++;

        if (startAt > 0 && moves <= startAt)
        {
                continue;
        }

        yield return enumerator.Current;
    }
    else
    {
        yield break;
    }
}

To:

while (TryMoveNext(enumerator, out enumerator))
{
    moves++;

    if (startAt > 0 && moves <= startAt)
    {
        continue;
    }

    yield return enumerator.Current;
}

You added the if for "boolean breaking", but at the same time you've subverted the inherent "boolean breaking" that a while loop provides.


Enumeration needed?

Since you are looking to repeat the same collection multiple times, this collection is cyclic in nature. Whenever I think about cyclic data, the modulo operator always comes to mind. It's by far the most common use case for the modulo operator.

According to this SO answer, though there may be a minor difference, the performance difference between enumeration and indexed arrays is reasonably negligible.
Keeping that in mind, I would advocate using a modulo operator simply because it keeps the code cleaner.

Note: The linked answer does not address re-enumeration, which is relevant for your code. But tbh, I'm not sure if re-enumeration is cached (thus not costing any meaningful overhead) or not. I'm going to continue my assumption that the performance differences are too small to be considered relevant for your current use case.

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int startPosition = 0)
{
    if (values == null)
    {
        throw new ArgumentNullException(nameof(values));
    }

    T[] array = values.ToArray();
    int count = values.Count();

    while (true)
    {
        yield return array[startPosition++ % count];
    }
}

I kept the example terse (e.g. by inlining startPosition++), but I hope it highlights the reduced complexity of the code in general.

A slight improvement would be to not let startPosition grow beyond the array's size:

    while (true)
    {
        startPosition = (++startPosition) % count;
        yield return array[startPosition];
    }

Though I think this is only really useful if you expect to enumerate up to a point of integer overflow, which I doubt is the case here.


Footnote

I'm torn about the startPosition variable name. Once we start using it as a counter, its name is no longer correct (it's the current position). However, I do think it fits as a parameter name.
We can assign the value to a variable with a better name; but I'm not sure if it's really that necessary for such a terse piece of code.

I can see arguments for either case. I'm unable to make a conclusive decision on whether it's necessary. I'm open to feedback on this.

If you do want to make the name more appropriate, I would suggest the following:

public static IEnumerable<T> Loop<T>(this IEnumerable<T> values, int? startPosition = null)
{
     //...

     int counter = startPosition ?? 0;

     //...
}
\$\endgroup\$
  • \$\begingroup\$ Oooh, crap, now this seems to be so obvious ;-) \$\endgroup\$ – t3chb0t Dec 4 '17 at 12:31
  • \$\begingroup\$ @t3chb0t: Such is the life of a programmer ;) I also updated the answer to reflect what I think is a more pertinent optimization (the one for which I first needed to understand the problem you were trying to fix) \$\endgroup\$ – Flater Dec 4 '17 at 14:45
  • \$\begingroup\$ This is clever and it made me think of one more scenario... it's not longer lazy because it dumps the source. I didn't think of it before... or maybe subconciously ;-] I think it'd better to use .ToArray().Loop() in this way rather than inside the extension and keep it lazy and consistant with other extensions. But I admit, it's not easy to pick one as there are good points for either approach. \$\endgroup\$ – t3chb0t Dec 4 '17 at 14:58
  • \$\begingroup\$ @t3chb0t: Keep in mind you'd need to change the parameter to T[] values for that optimization. But I think this is getting to a point of a style distinction, rather than a meaningful code optimization :) I favor IEnumerable as a parameter because I really dislike manually casting things; but you might prefer more direct control :) \$\endgroup\$ – Flater Dec 4 '17 at 15:01
  • \$\begingroup\$ Why not just name it position or pos instead of startPosition? In XML comments you can explain that the starting position should be passed, but it's reasonably explicit enough even without that. And then the name is accurate the whole way through. \$\endgroup\$ – Bobson Dec 4 '17 at 17:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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