5

I have the following code which a colleague of mine told me is incorrect and will crash if my variable is null:

List<FSKUser> users = null;

if (users == null || users.Count() == 0)
{
   return false;
}

Obviously the =null is just for testing purpose. But when I run this code it runs correctly and returns false.

Is the way I'm checking a safe and correct way to check?

Mike Kinghan
  • 55,740
  • 12
  • 153
  • 182
Zapnologica
  • 22,170
  • 44
  • 158
  • 253
  • Take a look at http://stackoverflow.com/questions/8582344/does-c-sharp-have-isnullorempty-for-list-ienumerable and http://stackoverflow.com/questions/5047349/how-to-check-if-ienumerable-is-null-or-empty – Soner Gönül Mar 31 '14 at 07:10

4 Answers4

10

Yes, this is the safe and correct way. Your colleague probably has some weird understanding of operator precedence or boolean expression evaluation in C# :)

The || operator (the same as &&) will stop evaluating as soon as it can be sure of the results. Since boolean OR can never yield false as soon as one of the operands is true, it will either fail on the first operand (if it's null, the result is true => you're done), or it will evaluate both of the operators.

Of course, if you're not averse to using extension methods, this can be handily used to simplify the condition. Eg. you can use an extension method like this:

public static bool IsEmpty<T>(List<T> @this)
{
  return @this == null || @this.Count == 0;
}

Which then lets you use a condition like this:

if (users.IsEmpty())
{
  ...
}

Also, note that List<T> has a Count property - you should probably use that instead of the extension method Count(). In the end, it will do the same thing IIRC (it checks if the enumerable is a collection or a list, IIRC), but it goes through some loops to do that.

You might want to ask your colleague what he thinks will happen. You've got a simple test case that shows you're right, but perhaps he's got some reasons of his own why he doesn't want that. The most likely thing, however, is that he's used to a different programming language, and not actually a native C#-er. In that case, you've both gotten an opportunity to learn :)

Luaan
  • 62,244
  • 7
  • 97
  • 116
  • 1
    I wanted to say he should use the `.Count` property (no parentheses) as well. It is fast with a `List<>`. The Linq extension will discover that this is an `ICollection<>` and use the `Count` property as well. However, if you want Linq, use `.Any()` instead since it is shorter, more precise, and can be faster when the underlying source is **not** a `List<>`. – Jeppe Stig Nielsen Mar 31 '14 at 07:46
2

Your friend is wrong. The || operator short-circuits - it exits with true the first time any term returns true. Similarly, the && operator exits with false the first time any term returns false. As such, your users.Count() cannot be reached if users is null (unless, perhaps, users is a field and you are doing lots of threading, and the compiler and JIT both choose to explicitly load the field twice for some reason).

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
0

There is no problem with your code. The 2nd condition will only be evaluated if users does not equal null, so this is the correct way to check against null.

In contrast to other languages (VB.NET and the And and Or operator as opposed to AndAlso and OrElse), C# only evaluates conditions as long as their result has an impact on the overall result. If - as in your case - the first condition of an Logical Or-operation already evaluates to true, there is no need to check the second one.

Markus
  • 20,838
  • 4
  • 31
  • 55
0

may be this would be useful in your example:

List<FSKUser> users = null;
return users != null && users.Any();
BigMan
  • 425
  • 5
  • 13