Flag empty collection or enumerate over members if collection is not empty (C#)

Question

The obvious solution (to me) is the following:

if (widgets.Count == 0)
{
    //Handle empty collection
}
else 
{
    // Handle non-empty collection
    foreach (widget in widgets)
    {
        // Process widget
    }
}

This seems indentatious. I am tempted to remove a level of indentation with "else foreach", but I can hear a low (but building) shriek in my head that starts immediately when I have that thought:

if (widgets.Count == 0)
{
    //Handle empty collection
}
else foreach (widget in widgets)
{
    // Process widget
}

Any other ideas or suggestions?

Personally I don't see anything wrong with either option, but you could have a flag that's set to false and set to true in the foreach. if it's still false after the foreach then the collection is empty. — D Stanley, Dec 21 '16 at 17:50
Possible duplicate of http://stackoverflow.com/questions/6547195/ — D Stanley, Dec 21 '16 at 17:52
Using Linq you could also try if(widgets.Any()) { foreach... — Kevin, Dec 21 '16 at 17:55
@Kevin you just moved the OP's `else` code into the `if` by negating the `if` condition. That does not solve the nesting problem. — CodeCaster, Dec 21 '16 at 18:02
You could also do sth. like implement an Extension-Method which gets two Action as Parameter. One handels empty List and the other filled List. Maybe a bit overkilled for this purpose but a way... — Sebi, Dec 21 '16 at 18:03
@CodeCaster, sorry didn't make the association between indentatious and nested. My answer was geared more to the issue in the title. Empty collection is already flagged (!x.Any()) and my suggestion enumerates the collection if it is not empty. — Kevin, Dec 21 '16 at 18:15
The answers I received are helpful: 1) "Don't fight conventions." 2) "When it starts looking complicated (or "indentatious"), break it out into independent functions. — David Rogers, Dec 29 '16 at 21:55

score 1 · Answer 1 · edited May 23 '17 at 11:46

You seem to be concerned with code readability and maintainability. Code blocks like this:

if (someCondition)
{
    // do
    // a
    // lot      
    {
        // moar
        // nesting
    }
}
else
{
    // do 
    // something
    // else
    {
        // possibly with even more nesting
    }
}

are generally considered neither readable nor maintainable. Sure, you can then abuse syntax or use other tricks to reduce the level of nesting, but the fact that you have so much nesting should be a dead giveaway that a method is doing too much.

At least extract the code blocks into their own methods, automatically reducing the nesting.

If you place the above code in its own method to begin with, you can return from the if(), removing the else block altogether.

public void DoSomethingWithWidgets(ICollection<Widget> widgets)
{
    if (widgets.Count == 0)
    {
        HandleEmptyCollection();
        return;
    }

    foreach (widget in widgets)
    {
        ProcessWidget(widget);
    }
}

Or instead put the code in their own classes altogether, and let a class determine for itself whether it applies to its inputs. You could implement that like this:

public interface IVisitor
{
    bool CanVisit(ICollection<Widget> widgets);

    void Visit(ICollection<Widget> widgets);
}

public class EmptyCollectionVisitor : IVisitor
{
    public bool CanVisit(ICollection<Widget> widgets)
    {
        return widgets.Count == 0;
    }

    public void Visit(ICollection<Widget> widgets)
    {
        // Handle empty collection
    }
}

public class NotEmptyCollectionVisitor : IVisitor
{
    public bool CanVisit(ICollection<Widget> widgets)
    {
        return widgets.Count > 0;
    }

    public void Visit(ICollection<Widget> widgets)
    {
        foreach (var widget in widgets)
        {
            // Process widget
        }
    }
}

Then you let all visitors visit the collection if they can, and you're done. With the added benefit that the classes themselves are better testable and more reusable.

You then utilize it by iterating over the registered visitors:

var yourInputCollection = { either a list that is empty or not };

IVisitor[] collectionVisitors = new[] { 
    new EmptyCollectionVisitor(), 
    new NotEmptyCollectionVisitor() 
};

foreach (var visitor in collectionVisitors)
{
    if (visitor.CanVisit(yourInputCollection))
    {
        visitor.Visit(yourInputCollection);
    }
}

Yes, this may seem like overkill, but abusing syntax to fit as much code as possible on as little horizontal space as possible ranks even lower in elegance on my scale.

looks interesting but i don't get how i can use this in practice. Can you give small sample how you call this at the end? — Sebi, Dec 21 '16 at 18:00

score 1 · Accepted Answer · answered Dec 21 '16 at 18:11

I have been tempted to use the pattern you're speaking of before in cases like this:

if (myVar.Value < 1)
{
    //...
}
else switch (myVar.Value)
{
    case 1:
        //...
        break;
    case 2:
        //...
        break;
}

The problem is the Visual Studio auto-indentation will constantly want to change it and indent your code after your else statement.

My advice would be use the first example in your question. Eliminating your else block does little for readability/maintainability.

score 1 · Answer 3 · answered Dec 22 '16 at 01:08

There's worse things in the world than one level of indentation, so I'd definitely prefer the former to the unusual style of the second.

Another approach again is to remove the syntactic sugar of the foreach:

using (var en = widgets.GetEnumerator())
{
  if (en.MoveNext())
  {
    do
    {
      var widget = en.Current;
      // process widget.
    } while (en.MoveNext());
  }
  else
  {
    // Handle empty.
  }
}

Syntactically it's worse, but it does have some advantages.

You can now use it with any IEnumerable<Widget> rather than having to insist on ICollection<Widget> which means you may be able to skip creating collections in memory just so you can see what the count is.
It's marginally more efficient that we skip the Count call and branch when we're going to (hidden in the foreach) find if it's empty in the first call to MoveNext() any way.

An analogous approach is very useful when you need to do something different for the first item (e.g. create an accumulator from the first item that an aggregating operation will then alter based on the rest).

That said, unless it was either in a hot path or the ability to use all enumerables was a real gain I'd just go with the first case in your question as the most conventional approach. If the indents are really that bad (because of the size of the code within them making it hard to read) you can always break that out into other methods.

score 1 · Answer 4 · answered Dec 22 '16 at 01:18

In your particular case, I'd probably opt for a helper variable that tracks whether the enumeration was empty.

bool empty = true;

foreach (widget in widgets) {
  empty = false;

  // Process widget
}

if (empty) {
  // Handle empty collection
}

This has the same advantage as Jon Hanna's answer of working on any IEnumerable<Widget> without enumerating twice, but in my opinion, in a far more readable form, and naturally avoids the need for one level of blocks.

Flag empty collection or enumerate over members if collection is not empty (C#)

4 Answers4