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?
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 :)
.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<>.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).
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.
Or is equivalent to C#'s | (single pipe, bitwise OR operator). Both C# and VB.NET have bitwise and short-circuiting logical operators.
AndAlso outnumbers And 100 to 1, and it's what automatically comes flowing out of my fingers by habit. If I were in the bad/incorrect habit of using | in C# where || is more appropriate, I could say that C#'s default behavior is not short circuiting either. It all comes down to appropriate choice of operators, not a language difference.may be this would be useful in your example:
List<FSKUser> users = null; return users != null && users.Any(); 