Looping over a list, checking a boolean and return value

Question

There are a number of different ways to do this. What do people prefer and why?

public boolean checkNameStartsWith(List<Foo> foos) {
    for (Foo foo : foos) {
        if (!(foo.getName().startsWith("bar"))) {
            return Boolean.FALSE;
        }
    }
    return Boolean.TRUE;
}

Answer 1

I'd change Boolean.FALSE to false and Boolean.TRUE to true. Using the constants objects from Boolean is rather unnecessary since the method returns with primitive boolean.

Answer 2

Though for loop is enough, you can rewrite using Guava if you like lambdas:

public boolean checkNameStartsWith(List<Foo> foos){
    return Iterables.all(foos, new Predicate<Foo>() {
        public boolean apply(Foo foo) {
            return foo.getName().startsWith("bar");
        }
    });
}

Answer 3

In addition to the other answers, I suggest adding a second parameter to the checkNameStartsWith method named prefix, e.g.

public boolean checkNameStartsWith(List<Foo> foos, final String prefix) {
    for (Foo foo : foos) {
        if (!foo.getName().startsWith(prefix)) {
            return false;
        }
    }
    return true;
}

Answer 4

I know it probably doesn't matter from a performance standpoint (due to branch prediction and compiler optimization), but I still prefer this form:

public boolean checkNameStartsWith(List<Foo> foos) {
    for (Foo foo : foos) {
        if (foo.getName().startsWith("bar")) continue;

        // Mismatch found
        return false; 
    }
    return true;
}

I think it's because it makes it easier to see that you're absolutely done with the iteration once you've determined there's no match. Otherwise, you might have something like this:

public boolean myMethod(List<Foo> foos, final String prefix) {
    for (Foo foo : foos) {
        if (!foo.getName().startsWith(prefix)) {
            // do something complex (many lines of code)
        }
        // here you could possibly do something else, regardless of condition true/false
    }
    return true;
}

You don't know unless you search for the end of the loop whether something is still done if the condition is false.

Answer 5

Boolean.FALSE and Boolean.TRUE should be definitely replaced with true and false because of auto-boxing. You don't need an additional Integer.intValue() call for each invocation.

I'm pretty sure Oracle HotSpot will optimize this automatically, but the code readability still suffers.

Answer 6

Have only one exit point from the method. Also, despite being old school, I would iterate using a while loop with a stop condition instead of the for-each.

public boolean checkNamesStartWith(List<Foo> foos, String prefix) {
  boolean ret = true;
  int index = 0;
  while (index < foos.size() && ret) {
    ret &= foos.get(index).getName().startsWith(prefix);
  }
  return ret;
}

Here the while loop will only iterate as long as it doesn't find a foo that starts with the prefix - or will iterate through the whole list. I would also rename the method from checkNameStart to checkNamesStart as it iterates through a whole collection, so usually more than one names will be checked.

Or, to preserve readibility, you can use an if condition with a break from the for-each loop:

public boolean checkNamesStartWith(List<Foo> foos, String prefix) {
  boolean ret = true;
  for (Foo foo : foos) {
    if (!foo.getName().startsWith(prefix)) {
      ret = false;
      break;
    }
  }
  return ret;
}

Answer 7

I would actually suggest you making a number of improvements:

1) Use an Iterator to walk over the list. Although this solution does not provide any performance benefits over the for loop, it actually makes your code more generalized and easier adaptable, should you sometime decide to use some other collection instead of a List.

2) Replace Boolean.TRUE and BOOLEAN.FALSE stuff by true and false respectively for reasons, explained by stoweesh.

3) Get rid of multiple returns. When you find an element in the list whose name does not start with bar, just set a boolean variable and break out of the loop. Return only in the end. This also improves readability and maintainability of your code.

My version of the iteration looks like this:

 public boolean checkNameStartsWith(List<Foo> foos) {

        Iterator<Foo> it = foos.iterator();
        boolean isAllStartWithBar = true;

        while (it.hasNext()) {
            if (!it.next().getName().startsWith("bar")) {
                isAllStartWithBar = false;
                break;
            }            
        }

        return isAllStartWithBar;   
    }