IEnumerable Extensions Linq AllOrDefault() Each()

Question

I recently decided to try and write my own implementation of Linq, which then lead me on to trying to solve some of the problems we have in our code base at work. Our code is littered with the following (obviously a simplified example):

        var items = new List<string> { "apple", "dog", "chair" };

        if (items != null && items.Any())
        {
            foreach (var item in items)
            {
                Console.WriteLine(item);
            }
        }

Essentially each time we are iterating over a collection we check to make sure the collection is not null and it actually contains at least one item.

I decided to write the following extension methods to alleviate this mess (the ML suffix on the methods was to differentiate from Linq [mostly for testing]):

        public static class EnumerableExtensions
        {
            public static IEnumerable<T> AllOrDefaultML<T>(this IEnumerable<T> items)
            {
                if (items.IsNullML()) return new List<T>();

                return items;
            }

            public static IEnumerable<T> AllOrDefaultML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
            {
                items = items.AllOrDefaultML();

                if (!items.AnyML()) return new List<T>();

                return items.Where(predicate);
            }

            public static IEnumerable<T> EachML<T>(this IEnumerable<T> items, Action<T> fn)
            {
                var e = items.GetEnumerator();
                while (e.MoveNext())
                {
                    fn(e.Current);
                }
                return items;
            }

            public static bool IsNullML(this object item)
            {
                return item == null;
            }

            public static bool IsNullOrEmptyML<T>(this IEnumerable<T> items)
            {
                return !items.AnyML();
            }

            public static bool AnyML<T>(this IEnumerable<T> items)
            {
                if (items.IsNullML()) return false;

                return items.Count() > 0;
            }

            public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
            {
                if (items == null) return false;

                items = items.Where(predicate);

                return items.Count() > 0;
            }
        }

Essentially with these methods I can write the above code as the following:

        var items = new List<string> { "apple", "dog", "chair" };

        foreach (var item in items.AllOrDefaultML())
        {
            Console.WriteLine(item);
        }

Or even integrate the Each() method:

        var items = new List<string> { "apple", "dog", "chair" };

        items.AllOrDefaultML().EachML(i => Console.WriteLine(i));

I'm not sure how I feel about the above, but you get the idea.

I've also added a predicate to AllOrDefaultML() which works if the sequence is null:

        foreach (var item in items.AllOrDefaultML(i => i.Length == 5))
        {
            Console.WriteLine(item);
        }

Linq has a FirstOrDefault() method, why not have an AllOrDefault() which returns you an empty sequence if it's null and doesn't have any items?

I'm hoping to get some insight in to the question above, and also wondering if anyone else has implemented anything like this before? Has it cleaned up your code much? Is it easier to read? Is it confusing for colleagues (particularly new start) who aren't totally aware of what's happening? Is there anything fundamentally wrong with what I have here?

Answer 1

Overall, the problem of having a collection, not knowing whether it's null, and wanting to treat a null collection the same as an empty one is probably a problem to be avoided, rather than solved. But that's beyond the scope of this question.

---

Generally this is well-written. You already mentioned the reason for the "ML" suffix, but to emphasize, it should definitely be removed.

For naming in general, I'd steer away from "OrDefault" for a few reasons:

• It's already used in FirstOrDefault and SingleOrDefault means "default" for an element, not a collection, so the connotations are a bit misleading
• The default keyword on a collection type would return null, not an empty collection, so again, the connotations aren't exactly what you want.
• It's quite cumbersome

Something as simple as .Safe() for your AllOrDefault method might be more appropriate.

Going by LINQ conventions, source is also a more idiomatic name than items.

---

EachML is quite weirdly implemented. Why not just use a foreach loop?

Alternatively, if you want to maintain laziness, you could do something like:

return items.Select(i =>
    {
        fn(i);
        return i;
    });

This then wouldn't be evaluated until the collection was enumerated over. However, I think that's probably more confusing/unexpected behaviour than the simple eager foreach.

---

Some of your methods seem a bit redundant. Do you really need a special IsNull method? Do you need IsNullOrEmptyML when you can just negate AnyML?

More generally, you can remove both those methods and your overloads which take predicates altogether. Then you can just do, e.g. collection.Safe().Any(predicate) rather than collection.AnyML(predicate). This saves you basically having to reimplement the majority of LINQ methods and just add in an extra null check.

---

So finally, with some radical pruning, I think everything there could be cut down to:

public static IEnumerable<T> Safe<T>(IEnumerable<T> source)
{
    return source ?? Enumerable.Empty<T>();
}

public static void ForEach<T>(IEnumerable<T> source, Action<T> action)
{
    foreach(var item in source)
    {
        action(source);
    }
}

Answer 2

In those places you use new List<T>(), I would recommend using Enumerable.Empty<T>() in its place. It's a little more idiomatic and, as those empty collections are cached by the runtime, repeated calls don't new up more stuff to be GC'd later.

items.Count() > 0 can be replaced with items.Any(), which is more descriptive of what you're trying to test for, and will likely be cheaper computationally. More specifically,

public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
{
    if (items == null) return false;

    items = items.Where(predicate);

    return items.Count() > 0;
}

can be replaced with

public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
{
    if (items == null) return false;

    return items.Any(predicate);
}

** EDITED TO ADD **

public static IEnumerable<T> EachML<T>(this IEnumerable<T> items, Action<T> fn)
{
    var e = items.GetEnumerator();
    while (e.MoveNext())
    {
        fn(e.Current);
    }
    return items;
}

has a small bug - items.GetEnumerator() returns an IEnumerator<T>, which implements IDisposable. This means you should Dispose() of it when you're done using it (see https://stackoverflow.com/a/232616/3312 for good info). Write it like this to do so:

    using (var e = items.GetEnumerator())
    {
        while (e.MoveNext())
        {
            fn(e.Current);
        }
    }
    return items;
}

Answer 3

I really don't think such extensions are necessary.

In your base case :

if (items != null && items.Any())
{
    foreach (var item in items)
    {
        Console.WriteLine(item);
    }
}

You don't (and shouldn't) check for Any. What's the worst case? You'll get in the foreach and do nothing since there are no elements in your list. This has an overhead.

AllOrDefault seems a little weird to me. I'm not sure it's useful but that's up to you. Though, to respect the LINQ's standards you should return a new instance of the list. Meaning :

public static IEnumerable<T> AllOrDefaultML<T>(this IEnumerable<T> items)
{
    if (items.IsNullML()) return new List<T>();

    return new List<T>(items);
}

AllOrDefaultML shouldn't exist. It's not clear what the function is used for. We're used to the Where syntax and you should keep these separated. That method has too many responsibilities : Check if it's null, check if there's something then filter.

Each is fine. It's exactly as ForEach for the List<>. But in that case, you don't check if the enumerable is null. You'll get a NullReferenceException in such a case. Also, I don't think it's useful for this method's return type to be anything else than void. But well, it could be a weird case of fluent API!

IsNull and IsNullOrEmpty are alright!

Why doesn't AnyML use Any? If it's to "rebuild" the LINQ extensions, you should consider not to use any of the LINQ's extension methods to learn better! (Using GetEnumerator and check if there's an element would be a good other solution)

Any with a predicate has a side effect you probably do not expect.

var list = new List<int>{1,2,3,4,5,6,7,8};
bool whatever = list.AnyML(i => i < 3);
Console.Write(list.Count);

What is the expected output of this? You'd expect the count to be 8. But no, the output is now 2. Because your AnyML method changes the list.

Then again, your Any method just uses other LINQ extensions to replace Any. Because the LINQ's Any already has an overload that takes a predicate.

Overall, your methods add an overhead to simple operations that are already done using LINQ. If you want to rewrite LINQ, don't use LINQ. :)

Answer 4

Seconding Ben Aaronson; usually a null collection is something that you want to avoid instead of attributing some kind of meaning to.

If the collection is a local variable:

• Why is it initialized to null? The only common scenario I can think of offhand where I would declare a collection variable and not initialize it would be when using a "TryGet"-style method where null indicates the state of an out parameter in an invalid scenario. But in that case I'd usually be checking the boolean result of the function, not whether the collection is null.

If the collection is a method parameter:

• Why does the null check exist?
• Does it make sense to pass null collections to this method or is this check a band-aid over existing code?
• Has the collection been previously mutated outside of the immediate scope of the caller--i.e., is the collection some global that is being initialized/set in other methods (usually private, void methods)? If so, this is a sign of "temporal coupling" and the conditional exists as a quick "bypass" for control flow. However it's not usually obvious to devs reading this code what is going on and whether or not these scenarios are actually supported--or worse, what a null collection is implying at this point.
• Can you guarantee that null always means the same thing? If no, how can you tell when it's a valid or invalid scenario? If yes, how did the collection get to be null in the first place? Is that an application flow that you want to support or is that indicative of a smell?

If the collection is a property/field on an object:

• Does it make sense for the object to be initialized with a null property? Why?
• Is some external code initializing the object and then setting the property? It's usually better to pass the collection to the object's constructor, or initialize the collection as an empty list that consumers can add to.
• Is this property being repeatedly overwritten in the class's methods? If so, see comments on temporal coupling.

In general, a collection should usually be at least initialized by the time it is accessed.