4

I saw this in code and was wondering if there is any performance benefit to checking the item count prior to looping:

if (SqlParams.Count > 0)
    foreach (var prm in SqlParams)
        cmd.Parameters.Add(prm);

I always prefer to do a null check instead and let the foreach loop just hop out if there are 0 items.

if (SqlParams != null)
    foreach (var prm in SqlParams)
        cmd.Parameters.Add(prm);

Isn't that the better way?

  • 4
    If you want to have an empty collection, use an empty collection, not null. – svick Apr 9 '14 at 17:29
  • 1
    @svick - I didn't mention anything about wanting an empty collection. – Code Maverick Apr 9 '14 at 18:21
  • My understanding is that the compiler generates the if > 0 check for you even if you don't supply one yourself. – Wyatt Barnett Apr 9 '14 at 20:41
  • 2
    Have you measured? What were the results? – Jesse C. Slicer Apr 9 '14 at 21:41
  • @JesseC.Slicer - I have not. I am in the middle of a project and don't have time to do so, that's why I thought I'd post it here to see if anyone already had done so. – Code Maverick Apr 9 '14 at 22:07
6
+50

Ultimately, the best answer is to actually test it. Make a method which loops over an empty array with and without checking the length first, call each 100,000 times and see which has a faster runtime.

public void withCheck(Integer[] array) {
    for (int i = 0; i < 100000; i++) {
        if (array.length > 0) {
            for (Integer i : array) {
                // nothing to do.
            }
        }
    }
}

public void withoutCheck(Integer[] array) {
    for (int i = 0; i < 100000; i++) {
        for (Integer i : array) {
            // nothing to do.
        }
    }
}

public void test() {
    long startTime = System.currentTimeMillis();
    withCheck();
    System.out.println("Time with check: " + (System.currentTimeMillis() - startTime) + "ms");
    startTime = System.currentTimeMillis();
    withoutCheck();
    System.out.println("Time without check: " + (System.currentTimeMillis() - startTime) + "ms");
}

When I write code like this, I generally ponder this as a trade-off between code readability and perceived or potential performance. In other words, adding a if (count > 0) check negatively impacts the readability, even though by a very small amount. So what is the gain? I presume there is very little or no advantage, but like you, I have not actually tested it. What I can tell you is that in all my years profiling, I have never found looping over an empty array to be a bottleneck.

Technically, it depends on how the iterator works. For a foreach loop like you have, you are creating an iterator object behind the scenes. My expectation would be, and again, this is unverified, is that the iterator object FOR AN ARRAY would simply be implemented like this (this is Java, but C# is probably comparable):

public class ArrayIterator<T> implements Iterator<T> {
    private int next = 0;
    public T next() {
        return backingArray[next++];
    }

    public boolean hasNext() {
        return next < backingArray.length;
    }
}

So iterating over such an iterator should exit out very quickly because hasNext() will return false. Notice it's the same check that your if statement would do.

Now if you are iterating over a linked list, the implementation should be different. If you explicitly check the size, you may be forcing it to scan the linked list. In Java, LinkedList.size() checks an explicit size variable so it doesn't walk the list, but I don't know how .NET implements it.

Another way to look at this is how likely is the collection to be empty? If that is a rare case, then if the theory that iterating over an empty array takes time is true, then adding an explicit check for empty first would only be faster if the collection is usually empty. If not, you are theoretically slowing down your average case just to slightly speed up your worst case.

Again, these are all theories and the timings are on the micro-level, so take it with a grain of salt.

TL;DR

It depends on the type of collection, the runtime platform, and the typical collection size, but intuitively, it seems unlikely to speed anything up in most situations. But the only way to know for sure is to test it.

  • 2
    Brandon's right. Michael Abrash (who's now at Oculus VR) wrote a book about code optimization years ago. Abrash is a genius. Anyway, his book basically says the question the OP is asking is important, but ASKING it is wasteful. There are too many variables that might affect whether the code will behave faster or slower IN A GIVEN CONTEXT. We don't write code in some theoretical world. We write code that ACTUALLY runs, against ACTUAL data. So as Brandon says, TEST IT. PROVE which one is faster, but don't treat your result like it's some "best practice". In a different context, it won't be. – Calphool Apr 16 '14 at 19:30
9

Not really, because if there are 0 items, the setup of the foreach loop will find that out anyway and simply not execute the body of the loop.

It's theoretically possible, in a very tight loop in which it's common for your collection to be empty, and in which finding the count is a very inexpensive operation, for there to be a noticeable performance benefit here, but if you're doing database queries, that's almost certainly not the case. So don't worry about it unless profiling shows that this is actually taking a significant amount of execution time.

4

Looking at the source code of System.Collections.Generic.List<T> we see that MoveNext() is implemented like this:

public bool MoveNext() {
    List<T> localList = list;

    if (version == localList._version && ((uint)index < (uint)localList._size)) 
    {                                                     
        current = localList._items[index];                    
        index++;
        return true;
    }
    return MoveNextRare();
}

Meaning that the total cost of one check is

  • Assigning an existing collection to another variable
  • Comparing versions, whatever that is
  • Casting two int values to uint and comparing them

When we look at the source code of .Count:

public int Count {
    get {
        lock (_root) { 
            return _list.Count; 
        }
    }
}

We see that the cost is acquiring a lock.

Conclusion

You can go measure and see which one performs best but I think it's safe to conclude that none of this will ever matter. You might have some uncommon collections to iterate over that don't do an early size check, but that's a very specific edge case that will point itself out then.

Bottom line: don't worry about it and you shouldn't bother adding a .Count > 0 or _collection != null check in the first place.

  • 2
    @JeroenVannevel a public method cannot safely assume a collection parameter won't be null. Therefore checking for null is important in this case. – Crono Apr 11 '14 at 19:16
  • 2
    @Doval As a rule of thumb, whenever I make a method public I assume even my grandma could use it. Therefore I always validate parameters. – Crono Apr 11 '14 at 19:32
  • 1
    I agree with @Crono. I don't ever make assumptions in my code. Especially in a team environment where other people have to access my methods. I want to make sure that whatever is passed in parameter wise is valid prior to executing the rest of the code. – Code Maverick Apr 11 '14 at 20:07
  • 1
    @CodeMaverick that's the right thing to do. I personally throw an ArgumentNullException exception for cases like this. This way the using developer has a clear explanation on why his code fails and doesn't need to go through my code to figure it out. – Crono Apr 11 '14 at 20:25
  • 2
    I think I explained it quite badly in my comment. What I meant by the program flow should've never gotten to that foreach with a null variable was that it shouldn't have been reached because at the start of the method you throw an exception when the argument was null. I also added make it an empty list which refers to inline initializing of a local variable (obviously this part wasn't conveyed well at all). Argument validation is definitely important and necessary, no argument there. – Jeroen Vannevel Apr 11 '14 at 20:28
3

The only scenario I can think of where checking Count first would help performance is with a collection having a resource-hungry IEnumerable.MoveNext() implementation and yet an highly efficient ICollection.Count implementation. While it's technically possible, it's very unlikely that it would ever happen.

In fact, on some scenarios, checking Count() first would actually be less performant. Think for example of ORM frameworks like Entity Framework which takes advantage of 's IQueryable interface. Behind the scenes, these objects reaches out to a datastore to provide collections of objects and computing operation results like sums, averages and of course count. Calling IQueryable.Count() here would actually end up making a SELECT COUNT(*) ... query on the database. If you don't need that information then it would just be needless overhead to get it.

Bottom line: I really wouldn't change anything to your actual coding style. It's perfectly acceptable to loop through a collection right away, even if it turns out to be empty. Making sure it isn't null first is okay if you cannot be sure of its provenance.

1

The benefit comes in the form of flexibility, but it has no performance benefit.

When a foreach runs over an ICollection with count of 0, the enumerator will register as having already traversed the whole set. Functionally, it's similar to the following for loop:

for(int i = 0; i > -1; i++)
    foo();

Now, if there is some other business rule that needs to fire prior to the foreach, that's a completely different story, and having the if condition accomodates that future capability nicely. But, it's also not strictly necessary, either.

EDIT: In response to your edit, the null check is a vastly better way to do it, since a collection can be null.

I wound up writing a IsNullOrEmpty extension method for IEnumerables, because I kept on seeing a lot of repeated conditions like:

if(SomeCollection != null
   && SomeCollection.Count > 0
   && (SomeOtherCondition))
{
    // Stuff that relies on SomeCollection having at least one value.
}
1

Short answer is: NO.

Details: SqlParameterCollection implements IEnumerable interface that allow you to use the foreach loop, the foreach itself insure that you don't get exception but in one condition that is System.NullReferenceException taking into your consideration that Null exception has nothing to do with (Count) property, you can have 0 params in the collection and the variable is initialized (not null).

So what do you need to do, simply check for null before the foreach syntax.

1

Consider system performance vs. a single execution. If the list is non-empty 99% of the time, then you are paying the cost of an extra check almost all the time.

On the other hand, if the list is empty 99% of the time, then it might work out differently (although I wonder if the extra invocation offsets any time you save -- remember we're talking about micro- or nano-seconds in this example).

So you can't just consider a single pass running in one thread, you have to consider the system's performance all together.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.