C# Code Simplification Query: The Null Container and the Foreach Loop

Question

I frequently have code that looks something like this:

if (itm != null)
{
    foreach (type x in itm.subItems())
    {
        //dostuff
    }
}
//do more stuff

In situations where //do more stuff is omitted, it is very easy to avoid the extra foreach loop. By exitting scope using the appropriate command (depending on what is going on, this generally would mean a return statement or a continue statement).

This type of thing tends to result in arrow code. I currently have a few ways to deal with this:

• Use code like itm = itm == null ? itm.subItems() : emptyArray
• Allow arrow code
• Use goto
• Use evil scoping hacks (wrapping the whole thing, if statement in all, in a scope and then breaking out of it). In my opinion evil scoping hacks are basically equivalent to goto except uglier and harder to read, so I don't consider this a valid solution.
• Refactor some of the chunks into new methods. There are in fact a few cases where this probably is a good solution, but mostly it's not appropriate since the null references are mainly error conditions from MS-functions.

Anyone care to offer a response on what approaches are considered preferable?

Answer 1

I like less nesting, for me it reads better. No goto please :)

I keep methods short, so it is usually a return for that scenario.

if (itm == null) return;
foreach (type x in itm.subItems())
{
   //dostuff
}

If the more stuff is needed, are simple statements and can be done before the foreach, you can:

if (itm == null)
{
   //do more stuff 
   return;
}
foreach (type x in itm.subItems())
{
   //dostuff
}

If the above is not the case, it is likely the method is too long and some of it would be moved away anyway. Probably:

if( itm != null ) SomeActionOnSubItems(itm.subItems);
// do more stuff (can be some method calls depending on level of abstraction).

Answer 2

First writing the code this way is very performant than most hacks. If you're looking for performance improvement don't bother.

If you're looking a way to accomplish this in a shorter way, depending on how big is foreach content you can do a:

if(itm != null)
{
  itm.SubItems().ForEach(x => do something with x ...);
}

Answer 3

Personally, I'd probably leave the structure the way you have it.

The first option (itm = itm == null ? itm.subItems() : emptyArray) seems less nasty than the others, but I still like your original better.

The problem is, from another developer's perspective, anything else is going to make your code less obvious. If there is a foreach running through a collection, I expect that the collection will (at least normally) have items contained in there. If the collection could be empty, that's not going to be obvious to other people without comments (which take longer to write than the if check).

Doing any of the hacks to avoid the if check just seems like you're trying to be too clever.

Answer 4

If you're using C# 3, you could always write an extension method:

public static IEnumerable<SubItem> SafeSubItems(this ItemType item)
{
     return item == null ? Enumerable.Empty<SubItem> : source.SubItems();
}

Then just write:

foreach (SubItem x in itm.SafeSubItems())
{
    // do stuff
}
// do more stuff

The key thing is that extension methods can be called even "on" null references.

What would be nice would be a "null-safe dereferencing" operator, so we could write:

// Not valid C# code!
foreach (SubItem x in itm?.SubItems() ?? Enumerable.Empty<SubItem>())
{
}

Or just define an EmptyIfNull extension method on IEnumerable<T> and use

// Not valid C# code!
foreach (SubItem x in (itm?.SubItems()).EmptyIfNull())
{
}

Answer 5

You could use the Coalesce operator (coded as a double question mark, ??, .net 2 upwards). This'll return the first non-null value in a list of values, so in this snippet...

MyClass o1 = null;
MyClass o2 = new MyClass ();
MyClass o3 = null;
return o1 ?? o2 ?? o3;

...o2 would be returned.

So you could re-code your original code sample as

foreach (type x in (itm ?? emptyArray).subItems())
{
    //dostuff
}

//do more stuff

However, personally I don't mind the nesting. It's instantly clear what's going on. I find the Coalesce operator a little harder to read, and that little nest is a small price to pay for clarity.