views:

192

answers:

5

I needed a method to give me all but the last item in a sequence. This is my current implementation:

    public static IEnumerable<T> SkipLast<T>(this IEnumerable<T> source)
    {
        using (IEnumerator<T> iterator = source.GetEnumerator())
        {
            if(iterator.MoveNext())
                while(true)
                {
                    var current = iterator.Current;
                    if(!iterator.MoveNext())
                        yield break;
                    yield return current;
                }
        }
    }

What I need it for is to do something with all the items except the last one. In my case I have a sequence of objects with various properties. I then order them by date, and then I need to do an adjustment to all of them except the most recent item (which would be the last one after ordering).

Thing is, I am not too into these enumerators and stuff yet and don't really have anyone here to ask either :p What I am wondering is if this is a good implementation, or if I have done a small or big blunder somewhere. Or if maybe this take on the problem is a weird one, etc.

I guess a more general implementation could have been an AllExceptMaxBy method. Since that is kind of what it is. The MoreLinq has a MaxBy and MinBy method and my method kind of need to do the same, but return every item except the maximum or minimum one.

+1  A: 

Your implementation looks perfectly fine to me - it's probably the way I would do it.

The only simplification I might suggest in relation to your situation is to order the list the other way round (i.e. ascending rather than descending). Although this may not be suitable in your code, it would allow you to simply use collection.Skip(1) to take all items except the most recent one.

If this isn't possible for reasons you haven't shown in your post, then your current implementation is no problem at all.

Noldorin
That is a good point yes. Actually didn't think about that... but I need the sequence ordered afterwards. which means I would have to order it and then reverse it again or something. could have worked too I guess... but feels a bit weird going back and forth like that :p
Svish
+6  A: 

This is tricky, as "last element" isn't a Markov stopping point: you can't tell that you've got to the last element until you try to get the next one. It's doable, but only if you don't mind permanently being "one element behind". That's basically what your current implementation does, and it looks okay, although I'd probably write it slightly differently.

An alternative approach would be to use foreach, always yielding the previously returned value unless you were at the first iteration:

public static IEnumerable<T> SkipLast<T>(this IEnumerable<T> source)
{
    T previous = default(T);
    bool first = true;
    foreach (T element in source)
    {
        if (!first)
        {
            yield return previous;
        }
        previous = element;
        first = false;
    }
}

Another option, closer to your code:

public static IEnumerable<T> SkipLast<T>(this IEnumerable<T> source)
{
    using (IEnumerator<T> iterator = source.GetEnumerator())
    {
        if(!iterator.MoveNext())
        {
            yield break;
        }
        T previous = iterator.Current;
        while (iterator.MoveNext())
        {
            yield return previous;
            previous = iterator.Current;
        }
    }
}

That avoids nesting quite as deeply (by doing an early exit if the sequence is empty) and it uses a "real" while condition instead of while(true)

Jon Skeet
What is a Markov stopping point? Anyways, good suggestions! The deep nesting and while(true) was some of the reasons why my solution felt a bit weird, hehe.
Svish
A: 

If you're using .NET 3.5, I guess you could use:

public static IEnumerable<T> SkipLast<T>(this IEnumerable<T> source)
{
  return source.TakeWhile((item, index) => index < source.Count() - 1))
}
Razzie
This enumerates over the collection twice however (unless sourcis actually a list), so it's probably a bad idea! (And won't even work the same in the general case of a non-deterministic iterator.)
Noldorin
It would also require Count() instead of Count. And actually, it would enumerate the collection many, many times - because the lambda expression would count the nodes every time it was evaluated!
Jon Skeet
for my own education: would it be ok if the count was done once outside the lambda? I don't quite understand why it would enumerate the collection twice.
Razzie
doing the count once before you start would be a lot better yes, but would still enumerate twice. once for the count and once for the taking.
Svish
(Changed Count to Count(), like Jon Skeet correctly noted :)
Svish
duh - for the count. Where's my coffee?!Anyway, I'll leave this answer here so other people don't make the same mistake :)
Razzie
Good idea. I didn't think about that lambda expression issue either, hehe.
Svish
A: 
public static IEnumerable<T> SkipLast<T>(this IEnumerable<T> source)
{
    if (!source.Any())
    {
     yield break;
    }
    Queue<T> items = new Queue<T>();
    items.Enqueue(source.First());
    foreach(T item in source.Skip(1))
    {
     yield return items.Dequeue();
     items.Enqueue(item);
    }
}
Handcraftsman
A: 

(Old answer scrapped; this code has been tested and works.) It prints
first
second
FIRST
SECOND
THIRD


public static class ExtNum{
  public static IEnumerable skipLast(this IEnumerable source){
    if ( ! source.Any())
      yield break;
    for (int i = 0 ; i <=source.Count()-2 ; i++ )
      yield return source.ElementAt(i);
    yield break;
  }
}
class Program
{
  static void Main( string[] args )
  {
    Queue qq = new Queue();
    qq.Enqueue("first");qq.Enqueue("second");qq.Enqueue("third");
    List lq = new List();
    lq.Add("FIRST"); lq.Add("SECOND"); lq.Add("THIRD"); lq.Add("FOURTH");
    foreach(string s1 in qq.skipLast())
      Console.WriteLine(s1);
    foreach ( string s2 in lq.skipLast())
      Console.WriteLine(s2);
  }
}
anon
There is no Count or indexer on an IEnumerable. So to do this you would have to put it into a list or something. And doing that means you would have to go through the elements twice. Once to put them into the list, and once to yield them.
Svish