Lazy man's IEnumerable extension verification method

Question

I often find myself doing this (or similar) with Lists and other collection types:

if (someList != null && someList.Count > 0)
    //take some action on the list

so I got lazy and wrote the following extension methods to do it for me:

    public static bool IsNullOrEmpty(this IEnumerable self)
    {   
        //Convert to collection here because IEnumerable doesn't have a count property
        ICollection c = self as ICollection; 
        return c == null || c.Count < 1;
    }

    public static bool IsNotNullOrEmpty(this IEnumerable self)
    {
        return !self.IsNullOrEmpty();
    }

    public static bool IsNotNullAndContains<T>(this IEnumerable<T> self, T item)
    {
        return self.IsNotNullOrEmpty() && self.Contains(item);
    }

Other than being lazy is there anything wrong with this or should I have just stuck with the manual check every time?

Edit:

Based on the answer from Jeroen Vannevel:

I originally had the first extension method (IsNullOrEmpty) defined as such:

public static bool IsNullOrEmpty<T>(this IEnumerable<T> self)
{
    return self == null || self.Count < 1;
}

which eliminated the need for the cast to ICollection, as IEnumerable<T> implements the count property. I removed the type parameter though because I wasn't actually using it for anything. Should I have just kept it like that to prevent the cast and a potential type cast exception?

Answer 1

It looks handy, but I have some thoughts on it.

Firstly: when you create your own IEnumerable<T> and don't implement ICollection, you'll get a logical error. It will try to convert it with as, which will return null because the conversion is not supported. Now your return statement c == null will return true and thus any collection that implements IEnumerable<T> but not ICollection will always be considered null or empty.

I don't know if there are collections in .NET that also have this scenario but you might want to check that as well.

Secondly: your first two methods perform a very closely related action and as such it is okay to combine these actions in one method. The third however does two things are not really similar to eachother and the guideline of "a method should do one thing" feels violated.

Generally: as soon as you add And to your methodname, it should be a red flag.

Lastly: An empty collection (aka: Count = 0) is not a problem for iteration. The iterator will break out of the code block very soon in the process so there is no performance gain by checking Count > 0. More reading material here.

Answer 2

In my opinion, there shouldn't be a need for this method to begin with. There are very few cases I can think of that would involve passing around a null IEnumerable (or List<T> for that matter). IEnumerable can still express a lack of things to iterate, so you don't often see IEnumerable members returning null. I often see null returned for IEnumerable when the better option would have been to fail or more explicitly inform the caller of an invalid operation. Take this Repository example:

public class Repository<T>
{
    public IEnumerable<T> GetItems()
    {
        bool canConnectToDb = false;
        bool recordsReturned = false;

        // Case 1:
        if(!canConnectToDb)
            return Enumerable.Empty<T>();

        // Case 2:
        if(!canConnectToDb)
            return null;

        // Case 3:
        if(!recordsReturned)
            return null;
    }
}

In "Case 1", the result is misleading. Sure, no records can be returned, but the reason isn't a lack of records to return. Instead, there is an error case that must be handled and an exception should be thrown. In "Case 2", null is being used to show an error state, which is misleading. What was the error? How can I differentiate between a null response, and error handling code using try..catch? In "Case 3", the correct return value would be an empty IEnumerable, not null.

Which leads me to your extension methods...Usually checks like these are trying to cover up issues rather than failing early and identifying them. For example, lets apply your extension method to refactor the existing Count() extension method:

public static int Count<T>(this IEnumerable<T> iterator)
{
    if(iterator.IsNullOrEmpty())
        return 0;
    else
        // Iterate and count...
}

You just end up perpetrating the issue to create "safe" code that really only hides the issue (that iterator shouldn't have been passed in as null to begin with). The actual implementation probably looks more like:

public static int Count<T>(this IEnumerable<T> iterator)
{
    if(iterator == null)
        throw new ArgumentException("iterator");

    // Iterate and count...
}

This allows it to fail early if it's improperly used, and doesn't make it this methods responsibility to make sure it can always execute given poor conditions.

Lastly, if you do plan on using something like your extension methods, you should know that the extension methods that come with the .NET framework already make performance checks similar to your check if the object is an ICollection. That said, there is no performance check they can do on a custom type that implements IEnumerable, so if you want the count the only option is to iterate through the whole IEnumerable. You don't really need the count though, you just need to know if there is anything being returned, which the Any() extension method does. Instead of iterating through the whole collection, it'll just iterate once and immediately return true if there is anything, which can be a huge performance saver.

A rework of your method:

public static bool IsNullOrEmpty(this IEnumerable iterator)
{
    return iterator == null || !iterator.Any();
}

Maybe I could be convinced by seeing a good application of this extension method, but I can't think of one.

Edit - To address the concern of using another library you can't entirely trust, you have some other options:

1) Use the Facade Pattern to create wrapper objects that do perform to your standards.

2) Treat the issue at the call site to these external libraries and not further into your own code. For example:

var items = BadLibrary.GetItems<T>() ?? Enumerable.Empty<T>();
InternalGoodMethod(items);

public static void InternalGoodMethod(IEnumerable<T> items)
{
    if(items == null)
        throw new ArgumentException("items");

    ....

Answer 3

public static bool IsNullOrEmpty(this IEnumerable self)
{   
    //Convert to collection here because IEnumerable doesn't have a count property
    ICollection c = self as ICollection; 
    return c == null || c.Count < 1;
}

As already mentioned, the typecasting to ICollection is probably problematic, take Enumerable.Range(0, 10); for example.

public static bool IsNullOrEmpty<T>(this IEnumerable<T> self)
{
    return self == null || self.Count < 1;
}

Edited : - This is wrong thanks to comment Perhaps I'm wrong, but self == null couldn't ever happen, if self == null a Object reference not set to an instance of an object exception will be thrown at the moment the method will be invoked on a null value.

IEnumerable objects, can also be implemented lazily, so the count will actually iterate over all of the Enumerable objects. I don't assume this is desirable for an 'All around' Null or empty check.

Anyway, different needs require different object handling, though I mostly agree with Ocelot20 answer.

• When returning an IEnumerable object, you should usually don't return a null value but an Enumerable.Empty();