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?

• Magus: If you want to use the .Count property, you should extend ICollection directly, or else this could fail on otherwise valid collections. Use the .Count() method in an IEnumerable extension. The second and third methods are a waste, however; particularly the simple inverted one.
• Brandon: I think the ICollection extension might be the better solution actually. Good idea.
• DarcyThomas: You shouldn't need to do the && someList.Count > 0 part. If you for each (etc) over an empty list, it will effectively do nothing and skip to the next part of you code. You will obviously need to check if it is null still (C#6 may have a solution to that coming soon as well)
• Dan Lyons: For IEnumerables, you could use the LINQ .Any() method instead of comparing .Count() with 0. Count forces enumeration of the entire collection, where an empty .Any() call just needs to touch one item.
• Brandon: IEnumerable does not expose Any(). IEnumerable<T> does.

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.

• Brandon: Thanks for the answer. A further question: Are you saying that my first two methods should only be one (leaving me to Not one method instead of calling another)? I have an update to add to my question if you could have a a look, regarding the ICollection business.
• Jeroen Vannevel: I was referring to the "null or empty" compared to "not null or empty and contains", but yes: a method that is simply an inversion of another doesn't really add much value. If the follow up question is short then you should ask clarification here in the comments, otherwise make a new post. This will keep things clear for future readers.
• Brandon: I was worried about the clarification, but it's directly related to your answer.
• Ocelot20: Not only does an inversion of another method not add any value, it violates the DRY principle (en.wikipedia.org/wiki/Don't_repeat_yourself) and creates an extra method to maintain/test/etc.
• MattDavey: "Firstly: when you create your own IEnumerable<T> and don't implement ICollection, you'll get a ClassCastException." I think you're getting confused between .NET and Java there. In .NET it's legal to have a class inherit IEnumerable but not ICollection.

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");
 ....

• Brandon: I like the Any() extension to this. There are very few cases I can see to pass around a null IEnumerable as well, but it happens. I'm working on a project with another developer where we've established a spoken rule: No instance of IEnumerable<T> will ever be null. With that said, another project I'm working on doesn't have that rule and the other devs pass around null List<T>s and IEnumerable<T>s like they're going out of style. I don't have a specific example of the use of this right now, but a lot of them exist.
• Brandon: Actually, I just tested: IEnumerable doesn't expose the Any() method.
• Ocelot20: See my update regarding the use of IEnumerable in other libraries you must use. Also, Any() is most definitely an extension method on IEnumerable shipped with the .NET framework. You may be missing a using System.Linq in your code. msdn.microsoft.com/en-us/library/bb534972.aspx
• Ocelot20: My apologies, I see you're using the non-generic IEnumerable interface. For that you can use a Cast<object> call or write the extension yourself if you are worried about performance. See here: stackoverflow.com/questions/5604168/count-for-ienumerable

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();

• NPSF3000: -1 "Perhaps I'm wrong, but self == null couldn't ever happen" - have you bothered to try it?
• NPSF3000: Np, downvote removed :)
• Brandon: For anyone wondering why self == null is incorrect (it took me a second to figure out what you were saying), the extension method is static and can be called on a null object.