1
\$\begingroup\$

What I mean by displacing an enumerable is skipping the first n items, then returning the next n items, then returning the first n items, then returning the rest of the enumerable.

What do you think? Does this idea have any value?

using System.Collections.Generic;

public class EnumerableDisplacer
{
    public virtual IEnumerable<T> Displace<T>(IEnumerable<T> source, int displaceCount)
    {
        var enumerator = source.GetEnumerator();
        var counter = 0;
        var displacedItems = new LinkedList<T>();
        while (counter < displaceCount)
        {
            enumerator.MoveNext();
            ++counter;

            displacedItems.AddLast(enumerator.Current);
        }

        counter = 0;
        while (counter < displaceCount)
        {
            enumerator.MoveNext();
            ++counter;

            yield return enumerator.Current;
        }

        foreach (var di in displacedItems)
        {
            yield return di;
        }

        while (enumerator.MoveNext())
        {
            yield return enumerator.Current;
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ A List would use less memory and almost certainly be faster than a LinkedList. \$\endgroup\$
    – Servy
    Dec 1, 2016 at 21:08
  • 4
    \$\begingroup\$ Do you have an idea what it is you think it's going to be useful for? \$\endgroup\$
    – forsvarir
    Dec 1, 2016 at 21:30
  • \$\begingroup\$ Can you provide sample inputs and outputs of your proposed Displace function \$\endgroup\$
    – Tolani
    Dec 1, 2016 at 22:44

2 Answers 2

1
\$\begingroup\$

Your implementation is missing some basic validation. A public method should always validate its passed in arguments. Assume an IEnumerable<int> will be passed containing 10 elements

with displaceCount == 10. You won't see an exception if it is a "true" IEnumerables<int> but you will receive wrong results.

A test with

IEnumerable<int> source = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
source = source.Where(i => i > 0);
EnumerableDisplacer dispatcher = new EnumerableDisplacer();
var result = dispatcher.Displace(source, 10).ToList();  

will result in a List<int> containing the 20 elements { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }

passing

IEnumerable<int> source = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
EnumerableDisplacer dispatcher = new EnumerableDisplacer();
var result = dispatcher.Displace(source, 10).ToList();    

will result in an InvalidOperationException in the second while loop at yield return enumerator.Current; because the enumeration has ended already.

You have two choices to do the input validation:

  • checking the count of items contained in source against displacedCount which will result for a "true" IEnumerable<T> in iterating over the elements twice

or

  • convert the source to an collection type like e.g an array and check its Length property against displacedCount which will result in iterating over the elements twice as well.

I would go with the second choice because it would make the whole method easier (IMO) because you wouldn't need any other datastructure like the LinkedList.

This would result in

public class EnumerableDisplacer
{

    public IEnumerable<T> Displace<T>(IEnumerable<T> source, int displaceCount)
    {
        if (source == null)
        {
            throw new ArgumentNullException("source");
        }

        var sourceArray = source.ToArray();
        var doubledDisplacedCount = displaceCount * 2;
        if (sourceArray.Length < doubledDisplacedCount)
        {
            throw new ArgumentOutOfRangeException("source", "The source needs to contain twice as many elements than displacedCount");
        }

        return DisplaceEx<T>(sourceArray, displaceCount);
    }
    private IEnumerable<T> DisplaceEx<T>(IEnumerable<T> source, int displaceCount)
    {

        var doubledDisplacedCount = displaceCount * 2;

        for (var i = displaceCount; i < doubledDisplacedCount; i++)
        {
            yield return sourceArray[i];
        }
        for (var i = 0; i < displaceCount; i++)
        {
            yield return sourceArray[i];
        }
        for (var i = doubledDisplacedCount; i < sourceArray.Length; i++)
        {
            yield return sourceArray[i];
        }

    }
}  

What advantage should this method get from beeing virtual ? I don't see any. Overriding this method in an inherited class doesn't make sense IMO.

\$\endgroup\$
9
  • \$\begingroup\$ While materializing the query makes it easier, it also removes functionality. It's opposed to a LINQ-esque style of programming where items in a sequence are deferred as much as possible, increasing the memory footprint and either performing work that may not be necessary, or at least performing work before it is necessary. The OP's approach provides that functionality (at the code of needing a bit more code to do so). All that needs to happen to deal with a smaller input sequence is to check the return value of MoveNext when iterating. \$\endgroup\$
    – Servy
    Dec 2, 2016 at 14:43
  • \$\begingroup\$ @Servy I basically agree, but assume the OP feeds a source containing 100.000 items with a displacedCount == 60.000 then a lot of work is done before it comes to MoveNext. And how should the OP deal with such a scenario. MoveNext would return false but a lot of items are yielded and maybe processed already. I know that my version isn't the best either, because it does the validation inside the "main" method instead of passing the validated parameters to the "main" method. (I will add this to my answer) \$\endgroup\$
    – Heslacher
    Dec 2, 2016 at 14:50
  • \$\begingroup\$ The solution is simply to replace the first two calls to enumerator.MoveNext() with if(!enumerator.MoveNext()) yield break; and you're done. \$\endgroup\$
    – Servy
    Dec 2, 2016 at 14:53
  • \$\begingroup\$ And then the caller of the code thinks that the method had succeeded which is wrong \$\endgroup\$
    – Heslacher
    Dec 2, 2016 at 14:55
  • \$\begingroup\$ Depends on what the expected behavior is. I don't see how the behavior that would result would be wrong at all, in the same way that if you call Skip(10) on a sequence with 5 items it just yields no items, it doesn't throw an exception. It doesn't need to. It has an entirely sensible behavior. The same is true of the OP's code. \$\endgroup\$
    – Servy
    Dec 2, 2016 at 14:57
0
\$\begingroup\$

Not sure what purpose you'd have for this. But using combinations of Skip Take and Concat, you could accomplish the same thing in one foreach block:

public virtual IEnumerable<T> Displace<T>(IEnumerable<T> source, int displaceCount)
{
    foreach(T t in source.Skip(displaceCount).Take(displaceCount).Concat(source.Take(displaceCount)).Concat(source.Skip(displaceCount * 2)))
    {
        yield return t;
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ You're iterating the source sequence multiple times here. That's not appropriate in a general purpose operation like this. The source sequence may not contain the same elements when iterated multiple times, it may cause side effects when iterated, or it may simply perform complex operations that shouldn't be performed multiple times. \$\endgroup\$
    – Servy
    Dec 1, 2016 at 22:17
  • 3
    \$\begingroup\$ On a side note, you really need to use more line breaks in your queries. \$\endgroup\$
    – Servy
    Dec 1, 2016 at 22:18

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.