2

Which is better?

List list = ... //get list from somewhere

for (int i=0; list != null && i < list.size(); i++){
    // ...
}

or?

List list = ... //get list from somewhere


if (list != null){
    for (int i = 0; i < list.size(); i++){
        // ...
    }
}

I got the idea from here: Scala or Java? Exploring myths and facts

Jagger
  • 10,350
  • 9
  • 51
  • 93
Muhammad Hewedy
  • 29,102
  • 44
  • 127
  • 219
  • 4
    If you control the code that returns the list, it would make your life easier to return an empty list instead of a null list. Then you won't need to answer the question any more. – assylias Jan 15 '13 at 18:28
  • I believe with Java 7+ you can use the `?` operator to check for null more succinctly. – BlackVegetable Jan 15 '13 at 18:31
  • 1
    @BlackVegetable Can you elaborate? AFAIK `?` can only be used as a wildcard in generics or in the `? :` ternary operators - both have been there since Java 1.5 – assylias Jan 15 '13 at 18:33
  • @assylias Doh! I thought it was a neat feature I was going to use when I upgraded but it turns out it was never accepted into the standard in the end! http://stackoverflow.com/questions/4390141/java-operator-for-checking-null-what-is-it-not-ternary – BlackVegetable Jan 15 '13 at 18:34
  • 1
    +1 to @assylias. Never using null collections, and using empty collections instead, is astronomically better than both of these. – Louis Wasserman Jan 15 '13 at 18:53
  • @BlackVegetable It's definitely not part of JDK 7 and I don't remember seeing it proposed for JDK 8. – assylias Jan 15 '13 at 18:53

5 Answers5

5

What do you mean with better? More readable code, or more efficient code? Anyway, I don't prefer any of your two proposals.

When I have to use a method, which returns a List, but possibly returns null (and I can't change the source code of that method to return an empty List instead) this is what I do:

List list = ... // get list from somewhere
if (list == null) {
    list = Collections.emptyList();
}
for (int i = 0; i < list.size(); i++) {
    // ...
}

IMHO this wins in terms of readability, performance and avoids deep nesting of our code.

jlordo
  • 37,490
  • 6
  • 58
  • 83
2

Leave the null check outside the loop.

Although you can save a line by merging it into the loop termination condition, no one does it like that, because you lose clarity:

You should only have iteration-related code in the loop code sections!

This is an elementary good coding style guideline.

Also, it is probably slightly slower to merge the check too, because you have to start the loop and execute the initialisation code even if list is null.

Finally, if you want to refactor to use the convenient foreach syntax, you can't merge the null check:

List<T> list;
if (list != null) {
    for (T t : list) {
        // ...
    }
{
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • This is the way to go. Anything else is worse imho. – szegedi Jan 15 '13 at 18:37
  • @szegedi: you think my answer is worse? – jlordo Jan 15 '13 at 18:48
  • @jlordo Actually it is because you are creating in fact an unnecessary instance of an empty list. – Jagger Jan 15 '13 at 19:01
  • @Jagger: Look at the `Collections` [source code](http://docjar.com/html/api/java/util/Collections.java.html). That instance exists as soon as the `Collections` class is loaded. It's just another reference to that instance ;) – jlordo Jan 15 '13 at 19:06
  • @jlordo First of all, I agree with assylias any code that returns null instead of an empty list is a bad idea generally. If I wrote the other code, I also wouldn't want to supress the other code's failure (if it was meant to return an empty list but because some inner problem it returned null). Also, (it's not specified whether he needs the iteration number or not, I know) I prefer the foreach loop. But there's nothing wrong with your answer, I should've used "I would certainly prefer this solution" in my comment, as the imho states, anything else is worse in readability for me. Cheers. – szegedi Jan 15 '13 at 20:31
1

Second one....In the first approach, each time in loop, unnecessary null check is happening

Renjith
  • 3,274
  • 19
  • 39
  • @Muhammad Performance wise there probably won't be any difference in the long run as the compiler should be smart enough to determine that the null condition can be extracted from the for loop... – assylias Jan 15 '13 at 18:31
  • I would expect the `JIT` to optimize the second code anyways. So, performance wise there won't be any difference AFAIK. – Rohit Jain Jan 15 '13 at 18:32
  • Yea...compiler might be optimizing..But we should not code thinking that compiler will optimize our code. – Renjith Jan 15 '13 at 18:39
  • @Renjith Exactly, we should code making sure that what we write is readable and maintainable. I happens in this case that it is also the more efficient solution. I'm just saying that in this case performance is not going to make a difference and should probably not be a criteria. – assylias Jan 15 '13 at 18:54
1

Neither of your proposal are efficient. I would suggest code similar to the below one (why make unnecessary method calls list.size() everytime):-

List list = ... //get list from somewhere

int size = (list==null ? 0 : list.size());

for (int i = 0; i < size; i++){
    // ...
}

This way the code is both readable and efficient.

Abhay Yadav
  • 88
  • 5
0

It would be the same in performance. For readability issues, I would prefer the second one. It is way more readable and clean for me.

Jorge Zuverza
  • 885
  • 8
  • 26