3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Overall, you should consider why you're in this situation where you're passing around so many collections without knowing whether they're null, and at the same time wanting to treat a null collection interchangeably with an empty one. \$\endgroup\$ – Ben Aaronson Nov 18 '15 at 17:49
  • \$\begingroup\$ @DarrenGourley, re-implementing some of the Linq extension methods is a good exercise. I learned a few things when I did it myself. \$\endgroup\$ – user2023861 Nov 18 '15 at 18:14
  • \$\begingroup\$ It is wasteful to check whether a collection contains items before iterating it. When you iterate an empty collection, the loop will execute zero times. If you have an array or list, you might get a benefit by checking the Length or Count property to avoid calling GetEnumerator and MoveNext, but you know what they say about microoptimization. If you have an enumerable, then for all nonempty collections, you're calling GetEnumerator twice and MoveNext twice for the first element. Just say "foreach" and let the body not execute when the collection is empty. \$\endgroup\$ – phoog Nov 18 '15 at 19:20
  • \$\begingroup\$ @phoog it was a bad / rushed example. Perhaps if I put a method about the foreach like DoSomethingMad() it would make more sense to check Any(). That's generally what's happening, not just the iteration over the list. For example, if there are Payments, process the order, then iterate over the payments to add them to my wallet. (again, an awful example :) \$\endgroup\$ – Darren Gourley Nov 18 '15 at 20:07
  • 1
    \$\begingroup\$ Fair enough. If I had a dime for each time I've seen if (xs.Any()) foreach (var x in xs) though ... \$\endgroup\$ – phoog Nov 18 '15 at 20:35
4
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Brilliant. Annoying... But brilliant! Glad I only spent a couple of lunch times on this now! You raise a very valid point about differentiating between null and empty lists, I'll have a look tomorrow for some real life examples. I think we have quite a few in our mvc views in cases where a list in the ViewModel may be null or empty (we didn't ask for it to populated, or there were no results). I'll have a play around with your idea of the safe method, and revisit some of these checks in our code to see if we can avoid it rather than try to fix it. Thanks for your time. It's much appreciated! \$\endgroup\$ – Darren Gourley Nov 18 '15 at 19:25
  • \$\begingroup\$ @DarrenGourley Deleting stuff is the best kind of redesign. The only reason you should be annoyed is because I had the fun of doing it rather than you :) \$\endgroup\$ – Ben Aaronson Nov 19 '15 at 17:34
4
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Fantastic! I did not know Enumerable.Empty<T>() was a thing. That was one of the big parts that was annoying me. Thanks for the spot, I used to have these going to a CountML() method, but quickly changed it for this question. \$\endgroup\$ – Darren Gourley Nov 18 '15 at 17:21
  • \$\begingroup\$ Glad to provide elucidation on that gem. I saw it used a couple years back and was gobsmacked at how often I'd use it. \$\endgroup\$ – Jesse C. Slicer Nov 18 '15 at 17:24
3
\$\begingroup\$

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. :)

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi @TopinFrassi. Thank you for taking the time to write your answer. I appreciate your feedback! The sample of code above is a cut down version of the Linq rewrite; admittedly there are some unintended errors as the original only used my methods and didn't reference linq at all. I didn't want to include everything as there was a lot and not to the point of the question. My attempted rewrite had other methods to replace select, take, etc. I quite liked the idea of the predicate in AllOrDefaultML(), kind of similar to how FirstOrDefault() works. Good spot with the AnyML() side effect. Thanks! \$\endgroup\$ – Darren Gourley Nov 18 '15 at 19:04
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hi @moarboilerplate. Thanks for taking the time to answer. Your questions and points you raised are good ones and I'll ask them to myself when I get around to refactoring this solution. Most of the collections are getter setter properties that don't get initialised on the constructor. Let's say for example we have an Order Class that contains ItemsOrdered and Payments (both lists) but no payments have been made yet, is it not ok for Payments to be null? Surely before you processed the order you would want to check if payments was null or empty? \$\endgroup\$ – Darren Gourley Nov 18 '15 at 20:01
  • \$\begingroup\$ @DarrenGourley well, if you did change the code so the collections were initialized via the constructors, you would make things easier on yourself. You now never have to check for null, and once something is throwing a null reference exception, it's a good thing because it's a sign that something is seriously wrong and the collection got completely overwritten. Taking it a step further, you can make the setter private so they can never be reassigned. You can add and remove items, but never set the lists directly. This will eliminate tons of edge cases (and probably large swaths of code) \$\endgroup\$ – moarboilerplate Nov 18 '15 at 21:14

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.