4

Edit: I made a mistake in my original question. It should be about methods Last and LastOrDefault (or Single and SingleOrDefault, or First and FirstOrDefault - plenty of them!).

Inspired by this question, I opened Reflector and looked at code of

Enumerable.Last<T>(this collection)

Then I jumped to code of

Enumerable.LastOrDefault<T>(this collection)

and I saw exactly the same piece of code (about 20 lines) differing in only one last line (first method returns default(T), second throws exception).

My question is why it is so? Why guys in Microsoft allow duplication of non-trivial pieces of code inside .Net framework? Don't they have code review?

Community
  • 1
  • 1
Konstantin Spirin
  • 20,609
  • 15
  • 72
  • 90
  • interesting, but no real question.... – Mitch Wheat Sep 05 '09 at 03:35
  • Also, Last(collection, predicate) has complexity of O(N) but that's probably topic for another discussion. – Konstantin Spirin Sep 05 '09 at 03:41
  • 2
    I think it is a real question. Maybe there's some good reason why it is that way? – Alex Baranosky Sep 05 '09 at 03:42
  • I believe `Enumerable.Last` must be in O(N) because `IEnumerator` can only be traversed sequentially starting at the first element. There is no O(1) method to get the last element. – bcat Sep 05 '09 at 03:48
  • I can see the methods differ significantly. What are you talking about? – Mehrdad Afshari Sep 05 '09 at 04:01
  • **@bcat**: If the IEnumerable<> is an IList<>, it's possible to directly get the last element. This is only possible, though, if there's no predicate. Interestingly, the framework does exactly this. – jpbochi Sep 05 '09 at 04:32
  • IEnumerables are not limited to being ILists, e.g., a generator. Last() is not only O(n); it can be undecidable. – Joe Chung Sep 05 '09 at 04:54
  • @jpbochi: Really? Cool, the framework is smarter than I thought! – bcat Sep 05 '09 at 05:00
  • I meant Last and LastOrDefault. Thanks for spotting a bug in my question - fixed now! – Konstantin Spirin Sep 05 '09 at 20:39
  • If the static type of the inner expression `x` is `IEnumerable`, then `x.Last()` is O(n). If the static type of the inner expression is `IList`, then `x.Last()` is O(1). – yfeldblum Sep 05 '09 at 20:43
  • 1
    Even with predicate it's possible to get O(1) (depends on predicate and collection, of course) by iterating from back to front and returning the first matching element. – Konstantin Spirin Sep 06 '09 at 03:10
  • 1
    @Konstantin - back to front and returning first matching element - O(n). Worst case you go through the whole input. – Kobi Sep 06 '09 at 11:56
  • @Kobi: Although technically still O(n), Konstantin's suggestion would have better performance than the current framework implementation in every situation except the worst-case. Worst-case performance would be the same as the framework implementation's best-case performance (ie, iterating through every item in the collection). – LukeH Sep 07 '09 at 16:30

1 Answers1

4

In fact, they are not quite the same. The first is like this:

public static TSource Last<TSource>(this IEnumerable<TSource> source)
{
    if (source == null) throw Error.ArgumentNull("source");

    IList<TSource> list = source as IList<TSource>;
    if (list != null) {
        int count = list.Count;
        if (count > 0) return list[count - 1];
    } else {
        using (IEnumerator<TSource> enumerator = source.GetEnumerator()) {
            if (enumerator.MoveNext()) {
                TSource current;
                do { current = enumerator.Current;}
                while (enumerator.MoveNext());

                return current;
            }
        }
    }
    throw Error.NoElements();
}

And the other is like this:

public static TSource Last<TSource>(this IEnumerable<TSource> source,
Func<TSource, bool> predicate)
{
    if (source == null) throw Error.ArgumentNull("source");
    if (predicate == null) throw Error.ArgumentNull("predicate");

    TSource last = default(TSource);
    bool foundOne = false;
    foreach (TSource value in source) {
        if (predicate(value)) {
            last = value;
            foundOne = true;
        }
    }
    if (!foundOne) throw Error.NoMatch();
    return last;
}

PS.: I hope I'm not breaking any copyrights now. :S

jpbochi
  • 4,366
  • 3
  • 34
  • 43
  • I meant Last and LastOrDefault. Thanks for spotting a bug in my question! – Konstantin Spirin Sep 05 '09 at 20:41
  • Considering the update, I see your point. The methods could be easily rewritten to avoid duplication. As for the reason they chosen not to do it, I have no idea. :S – jpbochi Sep 05 '09 at 22:48