Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
add comment

7 Answers

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.

share|improve this answer
    
I had got into the habit of always referencing the static constants as consumes less memory. Other people don't then ? –  NimChimpsky Nov 7 '12 at 11:04
6  
a reference to an object consuming less memory then a primitive? Do you have tests verifying that? Even if it is true, did you actually encounter a case where it did matter? –  Jens Schauder Nov 7 '12 at 11:19
    
@NimChimpsky: Using Boolean.FALSE and TRUE is better than creating new objects with new Boolean(false) but here the return type is boolean primitive, not an object reference. –  palacsint Nov 7 '12 at 11:27
    
@JensSchauder a reference to lots and lots of primitives across my whole application, consumes less memory than one Object reference, was my thinking. –  NimChimpsky Nov 7 '12 at 11:39
7  
there are no references to primitives. Just references or primitives. In case of a local boolean the difference surely is neglectable. If you have huge volumes of those (like huge Arrays) one should do some tests. For everything else, use the one easiest on the eye. –  Jens Schauder Nov 7 '12 at 11:49
add comment

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");
        }
    });
}
share|improve this answer
add comment

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;
}
share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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;
}
share|improve this answer
add comment

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;   
    }
share|improve this answer
    
it actually makes your code more generalized and easier adaptable really? foreach loop supports arrays, but your approach cannot be adopted to arrays. Also, I'd recommend use an iterator instead of foreach only if you need to remove elements. –  tcb Jun 28 '13 at 20:04
    
@tcb well, he did not say anything about arrays anyway, code features a List. And iterators are usable with arrays, just transform array into List by calling Arrays.asList. By more generalized I mean easier to adopt, if you decide to change the data structure, say use a LinkedList, ArrayList, some custom collection, etc. instead of List making your code polimorphic. Iterator enables you not only safely remove elements from the collection, but also supports paralelly traversing multiple collections, if you need. For these reasons I generally prefer to use an iterator rather than a foor loop. –  akhilless Jun 29 '13 at 11:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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