Is calling isEmpty() before iterating redundant?

Question

Given Collection<Object> foo, I usually do this check before iterating over it:

    if (foo != null && !foo.isEmpty()) {
        for (Object f : foo) {
            // ...
        }
    }

I'm just wondering, is this considered best practice? Shouldn't the isEmpty() check be redundant, since for() will just just ignore an empty collection without throwing errors?

How about the null check -- is that neccessary or is there a way to make for() simply ignore null Collections? (Without try-catching the for-loop)

I never check for `isEmpty()` while iterating and I've had no problems so far. — DCON, May 08 '17 at 16:17
Possible duplicate of [Is there a way to avoid null check before the for-each loop iteration starts?](http://stackoverflow.com/questions/6077909/is-there-a-way-to-avoid-null-check-before-the-for-each-loop-iteration-starts) — riddle_me_this, May 08 '17 at 16:19
Ideally, your collection should never be null and thus won't require the need for a null check. — riddle_me_this, May 08 '17 at 16:21
checking for `isEmpty` could avoid creating an Interator, but that is just micro-optimization and will cause nearly no impact at all. — user85421, May 08 '17 at 16:26

score 5 · Accepted Answer · answered May 08 '17 at 16:19

Shouldn't the isEmpty() check be redundant, since for() will just just ignore an empty collection without throwing errors?

It is redundant, it's completely harmless to use an enhanced for on an empty collection. Note that doing so does create an iterator object, so perhaps the check offers some very minor memory churn protection, but typically if that would be worth it, you'd have larger problems to solve.

How about the null check -- is that neccessary or is there a way to make for() simply ignore null Collections? (Without try-catching the for-loop)

You need the null check unless you were going to catch the NPE (which you said you didn't want to do) or you were going to allow that NPE to propagate to the caller (e.g., because foo should never be null at that point logically, or similar).

score 0 · Answer 2 · answered May 08 '17 at 16:19

0

With regards to the call to isEmpty I can assure you that is redundant. You might even want to check Joshua's Effective Java 2nd and revisit the code which translates from the for each loops.

answered May 08 '17 at 16:19

João Rebelo

79
14

score 0 · Answer 3 · answered May 08 '17 at 16:20

0

Yes, it is.

Only foo != null may be needed if can be null at that point of the code.

answered May 08 '17 at 16:20

LuisGP

431
3
13

Vaibhav Jain · Answer 4 · 2017-05-08T16:29:30.950

0

One of the best practice which I recently came across is to use CollectionUtils

if (org.apache.commons.collections.CollectionUtils.isNotEmpty(collectionName)) {
//Iterate through collectionName
}

Apache Commons' CollectionUtils.isNotEmpty(Collection) is a NULL-SAFE check

Returns TRUE is the Collection/List is not-empty and not-null Returns FALSE if the Collection is Null

Take below Example:

List<String> properties = new ArrayList();
if (CollectionUtils.isNotEmpty(properties)) {
  // process the list
} else {
 // list is null or empty
}

jar required: commons.collections.xxx.jar

edited May 08 '17 at 16:29

answered May 08 '17 at 16:21

Vaibhav Jain

1,939
26
36

How is this different/better than checking `isEmpty()`? – user2015253 May 08 '17 at 16:22
It requires an additional 3rd party library :-) – GhostCat May 08 '17 at 16:24
@user2015253 that requirement of additional null check is gone where we check if by chance list is coming as null. It will through null pointer exception. But in above case if list is coming as empty it will simple return false. Code looks more readable. – Vaibhav Jain May 08 '17 at 16:26

score 0 · Answer 5 · answered May 08 '17 at 16:21

0

Check for a null collection

No, it is not redundant. You must do it to avoid an NPE.
Check for an empty collection

Yes, it is redundant. No need to do it.

answered May 08 '17 at 16:21

VHS

9,534
3
19
43

score 0 · Answer 6 · edited May 23 '17 at 11:54

I don't see any use in the .isEmpty condtion, it just adds noise to the code making the actual functionality of the code harder to read identify.

How about the null check -- is that neccessary or is there a way to make for() simply ignore null Collections? (Without try-catching the for-loop)

You should always check for null before for-each loops. That is the only sollution you should consider because catching NPE (and RuntimeExceptions in general) is a bad parctice. Run-time exceptions are meant to signal a condition that wasn't anticipated at design time and frequently this means a programming mistake. So caching such exceptions can hide the real bugs in your code. For instance, if you surround the for-each with a try catching NullPointerException, how will you know that the exception occurred because the collection was null and not because of the code inside your loop.

score -1 · Answer 7 · edited May 09 '17 at 07:42

visibleTiles.isEmpty()

just checks whether map is empty (or) it has any elements.

ile = visibleTiles.keySet().iterator().next();

retrieves next element from iterator.

You need to do hasNext() check before doing next() on iterator.

hasNext() javadoc says

Returns true if the iteration has more elements. (In other words, returns true if next would return an element rather than throwing an exception.) so, if no element is available and call next() on iterator will return NoSuchElementException.

On top of this, I think you really would like to do NOT empty check

while (!visibleTiles.isEmpty())) {
    ....
}

Is calling isEmpty() before iterating redundant?

7 Answers7

Linked