152

I often come across code like the following:

if ( items != null)
{
   foreach(T item in items)
   {
        //...
   }
}

Basically, the if condition ensures that foreach block will execute only if items is not null. I'm wondering if the if condition is really needed, or foreach will handle the case if items == null.

I mean, can I simply write

foreach(T item in items)
{
    //...
}

without worrying about whether items is null or not? Is the if condition superfluous? Or this depends on the type of items or maybe on T as well?

Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • 1
    Similar to http://stackoverflow.com/q/3088147/80161 and http://stackoverflow.com/a/11734449/80161 – Nathan Hartley Oct 09 '15 at 14:51
  • 1
    @kjbartel's answer (at " https://stackoverflow.com/a/32134295/401246 " is the best solution, because it doesn't: a) involve performance degradation of (even when not `null`) generalizing the whole loop to the LCD of `Enumerable` (as using `??` would), b) require adding an Extension Method to every Project, or c) require avoiding `null` `IEnumerable`s (Pffft! Puh-LEAZE! SMH.) to begin with (cuz, `null` means N/A, whereas empty list means, it's appl. but is currently, well, *empty*!, i.e. an Empl. could have Commissions that's N/A for non-Sales or empty for Sales when they haven't earned any). – Tom Apr 23 '19 at 23:33

13 Answers13

168

You still need to check if (items != null) otherwise you will get NullReferenceException. However you can do something like this:

List<string> items = null;  
foreach (var item in items ?? new List<string>())
{
    item.Dump();
}

but you might check performance of it. So I still prefer having if (items != null) first.

Based on Eric's Lippert suggestion I changed code to:

List<string> items = null;  
foreach (var item in items ?? Enumerable.Empty<string>())
{
    item.Dump();
}
Vlad Bezden
  • 83,883
  • 25
  • 248
  • 179
  • 40
    Cute idea; an empty array would be preferable because it consumes less memory and produces less memory pressure. Enumerable.Empty would be even more preferable because it caches the empty array it generates and re-uses it. – Eric Lippert Jun 23 '11 at 14:23
  • 7
    I expect the second code to be slower. It degenerates the sequence to an `IEnumerable` which in turn degrades to enumerator to an interface making iteration slower. My test showed a factor 5 degradation for iteration over an int array. – CodesInChaos Jun 23 '11 at 14:41
  • @CodeInChaos actually second code is faster. With Enumerable.Range(1, 1000) I got about 41% faster and with Enumerable.Range(1, 10000) I got about 7% faster – Vlad Bezden Jun 23 '11 at 15:07
  • 12
    @CodeInChaos: Do you typically find that the speed of enumerating an empty sequence is the performance bottleneck in your program? – Eric Lippert Jun 23 '11 at 15:19
  • 16
    It doesn't only reduce the enumeration speed of the empty sequence, but of the full sequence as well. And if the sequence is long enough it can matter. For most code we should choose the idiomatic code. But the two allocations you mentioned will be a performance problem in even fewer cases. – CodesInChaos Jun 23 '11 at 15:22
  • I must say I was a bit surprised by the implementation of `Enumerable.Empty()`! I thought it would just define its own `IEnumerator`-implementing class and return false in `MoveNext()` and be done with it. I'm surprised it allocates a `T[0]` and delegates off to its `IEnumerable` implementation. Why bother allocating an empty array at all, even if it is "cached"? – James Dunne Jun 23 '11 at 16:38
  • 19
    @CodeInChaos: Ah, I see your point now. When the compiler can detect that the "foreach" is iterating over a List or an array then it can optimize the foreach to use value-type enumerators or actually generate a "for" loop. When forced to enumerate over either a list *or the empty sequence* it has to go back to the "lowest common denominator" codegen, which can in some cases be slower and produce more memory pressure. This is a subtle but excellent point. Of course, the moral of the story is - as always - if you have a perf problem then profile it to find out what the real bottleneck is. – Eric Lippert Jun 23 '11 at 16:39
  • Furthermore, Enumerable.Empty looks to not be thread-safe! (from looking at it in Reflector, the 3.5SP1 framework). *runs off to test hypothesis* – James Dunne Jun 23 '11 at 16:46
  • @James How can it not be threadsafe? It might return two different instances of the array if called in quick succession, but that doesn't violate its contract. Or if it uses static initializers its automatically threadsafe and returns always the same instance. – CodesInChaos Jun 23 '11 at 16:55
  • 1
    @Eric yes that conversion of the list to `IEnumerable` was what I was talking about. And while I agree that we need to profile to find the bottleneck profiling doesn't tell us how to improve the performance. – CodesInChaos Jun 23 '11 at 17:03
  • @CodeInChaos: I said it appears to not be thread-safe, simply because of the unsynchronized static field initialization in the static property. It does not use a static initializer, which is also odd. I see now that it would simply allocate more than one array instance in the worst case, as you noted. Nevermind :) – James Dunne Jun 23 '11 at 17:23
  • 1
    @kjbartel's answer (at " https://stackoverflow.com/a/32134295/401246 " is the best solution, because it doesn't: a) involve performance degradation of (even when not `null`) generalizing the whole loop to the LCD of `Enumerable` (as using `??` would), b) require adding an Extension Method to every Project, or c) require avoiding `null` `IEnumerable`s (Pffft! Puh-LEAZE! SMH.) to begin with (cuz, `null` means N/A, whereas empty list means, it's appl. but is currently, well, *empty*!, i.e. an Empl. could have Commissions that's N/A for non-Sales or empty for Sales when they haven't earned any). – Tom Apr 23 '19 at 23:30
  • Array.Empty would be better than Enumerable.Empty as the compiler should still use a for loop rather than using an enumerator. – kjbartel May 26 '20 at 06:37
  • @Tom it allocates lambda – OwnageIsMagic Dec 18 '20 at 23:18
101

Using C# 6 you could use the new null conditional operator together with List<T>.ForEach(Action<T>) (or your own IEnumerable<T>.ForEach extension method).

List<string> items = null;
items?.ForEach(item =>
{
    // ...
});
Community
  • 1
  • 1
kjbartel
  • 10,381
  • 7
  • 45
  • 66
  • Elegant answer. Thanks! – Steve Feb 27 '16 at 20:26
  • 2
    This is the best solution, because it doesn't: a) involve performance degradation of (even when not `null`) generalizing the whole loop to the LCD of `Enumerable` (as using `??` would), b) require adding an Extension Method to every Project, or c) require avoiding `null` `IEnumerable`s (Pffft! Puh-LEAZE! SMH.) to begin with (cuz, `null` means N/A, whereas empty list means, it's appl. but is currently, well, *empty*!, i.e. an Empl. could have Commissions that's N/A for non-Sales or empty for Sales when they haven't earned any). – Tom Apr 23 '19 at 23:31
  • 10
    @Tom: It assumes that `items` is a `List` though, rather than just any `IEnumerable`. (Or have a custom extension method, which you've said you don't want there to be...) Additionally, I'd say it's really not worth adding **11 comments** all basically saying that you like a particular answer. – Jon Skeet Apr 24 '19 at 05:36
  • @JonSkeet: a) Good point. Also, I should've mentioned that most of the other Answers were posted years before the Null-Conditional Operator was added (although I did say "is" (not "was") the "best"). b) IMHO, yes, it is! ;P That's lobbying / marketing (to increase chance the best answer is read and reduce / increase points for the undeserving / deserving). Sure, people shoulda / woulda / coulda read *all* answers of *all* same / similar threads, but people are nowhere near that perfect. – Tom Apr 24 '19 at 18:20
  • 4
    @Tom: I would strongly discourage you from doing so in future. Imagine if everyone who disagreed with your comment then added their comments to *all of yours*. (Imagine I'd written my reply here but 11 times.) This is simply not a productive use of Stack Overflow. – Jon Skeet Apr 25 '19 at 05:44
  • 3
    I'd also assume there'd be a performance hit calling the delegate versus a standard `foreach`. Particularly for a List which I think gets converted to a `for` loop. – kjbartel Apr 26 '19 at 03:58
  • 1
    Be carefull to don't use that and prefer normal foreach if inside the ForEach you manipulate a database. Or you will receive following error : A second operation was started on this context before a previous operation completed – Hayha Jul 25 '22 at 14:39
38

The real takeaway here should be a sequence should almost never be null in the first place. Simply make it an invariant in all of your programs that if you have a sequence, it is never null. It is always initialized to be the empty sequence or some other genuine sequence.

If a sequence is never null then obviously you don't need to check it.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 2
    How about if you get the sequence from a WCF service? It might be null, right? – Nawaz Jun 23 '11 at 14:57
  • 4
    @Nawaz: If I had a WCF service that returned me null sequences intending them to be empty sequences then I would report that to them as a bug. That said: if you have to deal with badly formed output of arguably buggy services, then yes, you have to deal with it by checking for null. – Eric Lippert Jun 23 '11 at 15:20
  • 8
    Unless, of course, null and empty mean completely different things. Sometimes that's valid for sequences. – configurator Jun 23 '11 at 23:32
  • @Nawaz How about DataTable.Rows that returns null instead of an empty collection. Maybe that's a bug? – Neil B Jan 22 '19 at 00:26
  • @kjbartel's answer (at " https://stackoverflow.com/a/32134295/401246 " is the best solution, because it doesn't: a) involve performance degradation of (even when not `null`) generalizing the whole loop to the LCD of `Enumerable` (as using `??` would), b) require adding an Extension Method to every Project, or c) require avoiding `null` `IEnumerable`s (Pffft! Puh-LEAZE! SMH.) to begin with (cuz, `null` means N/A, whereas empty list means, it's appl. but is currently, well, *empty*!, i.e. an Empl. could have Commissions that's N/A for non-Sales or empty for Sales when they haven't earned any). – Tom Apr 23 '19 at 23:32
  • This seems bizarre to me, why would you declare that all collections everywhere in all software cannot return null, just because foreach handles null badly? 99% of the time I'm using a collection, I'm either using LINQ where I can ignore nulls with `?.` or I'm looping through it with a foreach loop which throws an exception on null. I'd much rather foreach loops just skip over null collections and if I need to check for null because it's a problem I'll do so explicitly. – Jason Masters Sep 09 '19 at 20:44
  • @JasonMasters: You say that I make this claim **just** because foreach handles nulls badly. But that is only one reason of many why this is a bad practice. **Ignoring nulls hides bugs.** Remember, the design principle of C# is "if the program looks wrong, do not muddle on through and hope for the best; fail quickly to alert the developer that their program is wrong". If you don't like that principle, C# might not be the language for you; try Visual Basic or JavaScript if you want a language that cleaves to the principle "we should guess what the developer probably meant, and fail silently." – Eric Lippert Sep 10 '19 at 05:59
  • @Eric Lippert, that's the old argument that's always trotted out, but a lot of modern programming is web connected and based on communication between decoupled microservices, in which your communication is inherently fuzzy. Forcing constant null checks and making it a pain in the a** to ignore nulls makes C# badly suited for *any* web development. Not to mention that the creators added `?.` and `??` null conditional operators specifically to easily ignore nulls. imho the foreach loop's null behaviour is more a relic then bad practice. – Jason Masters Sep 11 '19 at 22:52
  • 1
    @JasonMasters: And then why is C# 8 doing an ENORMOUS amount of work to make sure that you can express "this reference is never null" in the language and framework? The notion that you need null checks is where things have gone off the rails: **if references are never null then you don't need null checks**. That's the correct solution: ban null references. Not encourage them! It is precisely that attitude that *requires* so many null checks. – Eric Lippert Sep 11 '19 at 22:56
  • @JasonMasters: Now, if you're going to make the argument that *null reference semantics should be lifted semantics* the same way that null arithmetic semantics are lifted semantics, and therefore null calls should not crash but silently fail over into nulls, just as arithmetic silently fails over into nulls, then yeah, I would have made the same argument in 2001. But we are unfortunately having to tame a world in which null references are rampant and do crash by default. Don't make them not crash; crashing is what helps us find and destroy them. – Eric Lippert Sep 11 '19 at 22:58
  • @EricLippert What about `IEnumerable` where execution is intended to be deferred ? If our logic first loads a collection from persistent storage or retrieves it from a remote api, next manipulates the collection ( LINQ filter/sort/transform ) and finally we use the actual data in some manner ( iterate the collection and compose some html ) but at the point of iteration we demand that the collection not be null, then in the manipulation logic aren't we in the position of switching the `IEnumerable` to `List`which prematurely forces evaluation ? – BaltoStar Nov 26 '19 at 20:13
11

Actually there is a feature request on that here: https://github.com/dotnet/csharplang/discussions/1081#issuecomment-443209795 And the response is quite logical:

I think that most foreach loops are written with the intent of iterating a non-null collection. If you try iterating through null you should get your exception, so that you can fix your code.

StayOnTarget
  • 11,743
  • 10
  • 52
  • 81
Teoman Soygul
  • 25,584
  • 6
  • 69
  • 80
  • I guess there are pros and cons for this, so they decided to keep it as it was designed in the first place. after all, the foreach is just some syntactic sugar. if you'd have called items.GetEnumerator() that would have crashed also if items was null, so you had to test that first. – Marius Bancila Jun 23 '11 at 14:10
6

You could always test it out with a null list... but this is what I found on the msdn website

foreach-statement:
    foreach   (   type   identifier   in   expression   )   embedded-statement 

If expression has the value null, a System.NullReferenceException is thrown.

BoltClock
  • 700,868
  • 160
  • 1,392
  • 1,356
nbz
  • 3,806
  • 3
  • 28
  • 56
3

You can encapsulate the null check in an extension method and use a lambda:

public static class EnumerableExtensions {
  public static void ForEach<T>(this IEnumerable<T> self, Action<T> action) {
    if (self != null) {
      foreach (var element in self) {
        action(element);
      }
    }
  }
}

The code becomes:

items.ForEach(item => { 
  ...
});

If can be even more concise if you want to just call a method that takes an item and returns void:

items.ForEach(MethodThatTakesAnItem);
Jordão
  • 55,340
  • 13
  • 112
  • 144
2

It is not superflous. At runtime items will be casted to an IEnumerable and its GetEnumerator method will be called. That will cause a dereferencing of items that will fail

boca
  • 2,352
  • 19
  • 21
  • 2
    1) The sequence will not necessarily be cast to `IEnumerable` and 2) It's a design decision to make it throw. C# could easily insert that `null` check if the developers considered that a good idea. – CodesInChaos Jun 23 '11 at 14:35
1

You do need this. You'll get an exception when foreach accesses the container to set up the iteration otherwise.

Under the covers, foreach uses an interface implemented on the collection class to perform the iteration. The generic equivalent interface is here.

The foreach statement of the C# language (for each in Visual Basic) hides the complexity of the enumerators. Therefore, using foreach is recommended instead of directly manipulating the enumerator.

Steve Townsend
  • 53,498
  • 9
  • 91
  • 140
  • 1
    Just as a note it technically doesn't use the interface, it uses duck typing: http://blogs.msdn.com/b/kcwalina/archive/2007/07/18/ducknotation.aspx the interfaces ensure that the right methods and properties are there though, and aid understanding of intent. as well as use outside foreach... – ShuggyCoUk Jun 23 '11 at 14:57
0

The test is necessary, because if the collection is null, foreach will throw a NullReferenceException. It's actually quite simple to try it out.

List<string> items = null;
foreach(var item in items)
{
   Console.WriteLine(item);
}
Marius Bancila
  • 16,053
  • 9
  • 49
  • 91
0

the second will throw a NullReferenceException with the message Object reference not set to an instance of an object.

harryovers
  • 3,087
  • 2
  • 34
  • 56
0

As mentioned here you need to check is it not null.

Do not use an expression that evaluates to null.

Renatas M.
  • 11,694
  • 1
  • 43
  • 62
0

The accepted answer is getting old. Nowadays, nullable types are used extensively and help the compiler understand what you're trying to achive (and avoid mistakes).

Which means that your list could be this :

List<Item>? list

...OR... this :

List<Item> list

You'll need to check for nullability only for the former case.

Same thing goes for items:

List<Item?> list

...OR... this :

List<Item> list

You'll need to check for nullability of items only for the former case.

And of course finally you have this :

List<Item?>? list

where anything (list and items) could potentially be null.

==================

EDIT: A picture is better than 1,000 words

enter image description here

enter image description here

enter image description here

jeancallisti
  • 1,046
  • 1
  • 11
  • 21
  • _"You'll need to check for nullability only for the former case."_ ... You mean `List list = null` is a compiler error now and isn't a possible scenario? – Nawaz Nov 28 '22 at 14:32
  • Mostly yes. _Technically_ no, because of course there are still ways to force a null value at runtime if you try really hard (e.g. if you use Json.Net which was written before nullable objects and doesn't know the difference between `string` and `string?` ) but I guarantee you that using `?` removes 99% of worries and the compiler will know its way around. If _you_ generated the objects and didn't introduce `?` anywhere then you're safe. If the objects come from an unknown source then use `[NotNull]` in critical places (I'll let you google that) but that falls under corner cases. – jeancallisti Nov 28 '22 at 15:35
  • Why one needs to find _"ways to force"_ a null value when one can simply write `List list = null`? Or simply do not initialize it at all. Isn't it `null` BY DEFAULT? Or, are you saying that the language has been changed in this regard and `List list` is NOT null by default? – Nawaz Nov 28 '22 at 18:35
  • "One can simply write List list = null , or not initialize it at all" - as I said before, the compiler doesn't let you, and you are mostly correct that you can't do that. Unless you have a weird compiler. Unlike Java, C# doesn't fool around with null in its recent versions (for quite a few years, now) – jeancallisti Nov 30 '22 at 08:40
  • I used .NET 7 and I'm able to do `List strings = null;` See it yourself: https://dotnetfiddle.net/udw3y7 ... Also, if it was once allowed, I don't think they have changed the language in this regard to disallow this. Because if they do so, that'd be a HUGE breaking change. `List` is a reference type and references can be `null`. – Nawaz Dec 01 '22 at 13:02
  • Well then it means that the language allows it but that Visual Studio is smarter than that. Get yourself a good IDE. Start by trying with VSCode and see if it lets you. Also why would you write "Non-Nullable Type" variable = null? Don't do that. Use question marks as often as possible and you'll be fine. – jeancallisti Dec 04 '22 at 11:07
  • @Nawaz read this : https://stackoverflow.com/questions/61627625 – jeancallisti Dec 04 '22 at 11:19
  • But the question is, what if `list` is `null`? The compiler does NOT give any guarantee that it _cannot_ be `null`. Hence this question. In order to write safe code, one needs to use `if` or equivalent, to check `list` is not null. – Nawaz Dec 05 '22 at 12:56
-1

In C# 6 you can write sth like this:

// some string from file or UI, i.e.:
// a) string s = "Hello, World!";
// b) string s = "";
// ...
var items = s?.Split(new char[] { ',', '!', ' ' }) ?? Enumerable.Empty<string>();  
foreach (var item in items)
{
    //..
}

It's basically Vlad Bezden's solution but using the ?? expression to always generate an array that is not null and therefore survives the foreach rather than having this check inside the foreach bracket.

dr. rAI
  • 1,773
  • 1
  • 12
  • 15