Java While-Loops

Question

So while rewriting some code, I came across something along the lines of:

Method 1

while ( iter.hasNext() ) {
    Object obj = iter.next();
    if ( obj instanceof Something ) {
        returnValue = (Something) obj;
        break;
    }
}

I re-wrote it as the following without much thought (the actual purpose of the re-write was for other logic in this method):

Method 2

while ( (iter.hasNext()) && (returnValue == null) ) {
    Object obj = iter.next();
    if ( obj instanceof Something ) {
        returnValue = (Something) obj;
    }
}

I personally don't have any strong preference for either and really don't see anything wrong with either approach. Can anyone else think of benefits or consequences of using either approach? The variable, returnValue is returned. How would people feel if that was the last block in the method, and it is just returned?

EDIT: So here's what I'm doing: Currently this method takes a set of authorizations and validates them - returning a boolean. This method allows grouping so you can specify that at least one or all (meaning if at least one authorization is valid, pass the whole set). However, this method does not support levels of authorization which is what I'm changing it to do so that each level can specify different grouping. All this bit is just background information...does not have that much to do with the above code - an alternative method is used to perform the above block of code.

Answer 1

One school of thought is that a method should only have one return statement/point at the end of the method.

I tend to use whatever is clearer in the specific circumstances.

As an aside:

1) Should you be rewriting code that works (assuming it does)?

2) Should you be rewriting code that doesn't have tests (assuming it does not)?

Answer 2

I like the first as it is more clear why you are breaking out of the loop. It says "if this condition is true, set the value and break the loop, we're done". The other took me another second or so, but as you said, there is no huge difference.

Answer 3

Those are equivalent in this case but that is no longer the case if you want to do something after the if condition, which is often the case.

Performance is a wash and even if it wasn't, it's an irrelevant micro-optimization. The key goal should be readability and maintainability.

Answer 4

I prefer the explicit jump (whether it's a break or a return). It's difficult for me to articulate why, but I can compare it to writing in active vs. passive voice. Sure, it's merely style, but active is a lot more straightforward to read.

Answer 5

Clearer, for me, would be to extract this as a method of its own; then you can simply return the value rather than assigning it to a local.

while (iter.hasNext()) {
    Object obj = iter.next();
    if (obj instanceof Something)
        return (Something)obj;
}
return null;

Better yet would be a foreach loop

for (Object o : yourList)
    if (o instanceof Something)
        return (Something)o
return null;

Answer 6

Method 1 is much clearer in what it is doing, it breaks the loop when obj instanceof Something==true, although method 2 is also pretty obvious. Another slight advantage of Method 1 is that it is slightly faster since method 2 does one more condition check every loop, and it does one more check than method 1. What if the check took more than 1 minute?

So obviously Method 1 is better and easier to understand.

Answer 7

Clearly the first form is much better because it is less complex. If you are not doing anything with returnValue you could simply return it as soon as you have found a match.

Answer 8

I don't agree that the first form is clearly better... The problem with returns, breaks and continues is that they make the code harder to refactor. The example given here is too simple to require such manipulation but in general it is all too easy to refactor a complex piece of code into its own method and neglect to ensure the caller's return paths remain valid. This is where automated testing can help, but I still prefer most of my checking to come out of the compiler.

I took the approach of asking what the loop is doing - Two things, it checks for the existence of an Object of type Fish, and it makes that Object available outside the loop. If we separate the two responsibilities we get:

Object obj = null;
while (iter.hasNext() && !(obj instanceof Fish)) {
  obj = iter.next();
}
if (obj instanceof Fish) {
  returnValue = (Fish) obj;
}

I still don't like it... perhaps its the instanceof or the use of Object or even just the nature of Iterator but it doesn't seem like nice code.

Answer 9

After your edit, it seems that you will have iterate through the whole list to check for all kinds of authorities. The code would be like:

Object returnValue=null; //Notice in your original code
while(iter.hasNext()){
    Object kind=iter.next()
    switch(kind.getType()){
       case "fish": 
       case "reptile":
          if(returnValue!=bird|returnValue!=mammal)
               returnValue=(coldblood) kind;
          break;
       case "bird":
       case "mammal":
          returnValue=(coldblood) kind;
          break;
       case default:
          //Fall-through
    }
}
return returnValue;

So this is what I have to say:

1. You said you needed to implement multiple levels of permissions, I believe that you will have to iterate through the whole collection.
2. That having said, I recommend using Method 1 - that is if you have to break. The break keyword is more clear, and it is slightly faster. (One less jump however slight)