CODEWITHSUNDEEP
javafor-loopintellij-idea
Koray Tugay
Koray Tugay

Reputation: 23780

IntelliJ complaining "for statement does not loop"?

This is the code I have:

public enum Modification {
    NONE, SET, REMOVE;
}

boolean foo(List<S> sList){
    for (S s : sList) {
        Modification modification = s.getModification();
        switch (modification) {
            case SET:
            case REMOVE:
                return true;
            /*
            case NONE:
                break;
            */
        }
    }
    return false;
}

And when the code is as seen above, IntelliJ will say:

'for' statement does not loop less... () Reports any instance of for, while and do statements whose bodies are guaranteed to execute at most once. Normally, this is an indication of a bug.

Only if I make the following change, IntelliJ will be happy:

for (S s : sList) {
    Modification modification = s.getModification();
    switch (modification) {
        case SET:
        case REMOVE:
            return true;
        case NONE:
            break;
    }
}

Why is my for loop not looping if case NONE: is not included in the switch statement?

Upvotes: 17

Views: 29152

Answers (5)

Philippe Banwarth
Philippe Banwarth

Reputation: 17725

I think that IntelliJ's inspections is wrong. I reported it to JetBrains

Edit : it's fixed

Upvotes: 1

Gerald M&#252;cke
Gerald M&#252;cke

Reputation: 11122

I'd say its a false positive.

1st indication: If you run your code through a debugger - and have elements with NONE modification in the list before an element with other modifications - it will actually loop.

2nd indication: When you look at the generated bytecode, it transforms the switch statement to (sort of - its not exactly the same)

for (S s : sList) {
    Modification modification = s.getModification();
        switch (modification.ordinal()) {
            case 1:
            case 2:
                return true;
    }
}

If you put that in your code, IntelliJ does not complain.

3rd indication: the warning dissappears if you put an additional statement before the return, i.e. System.out.println();

 switch (modification) {
     case SET:
     case REMOVE:
       System.out.println()
       return true;

Seems you tricked the inspection with the missing case label and could simply ignore the warning.

Upvotes: 1

Ben Thurley
Ben Thurley

Reputation: 7141

I just tried this in eclipse and you end up with a compiler warning on the switch statement.

The enum constant NONE needs a corresponding case label in this enum switch on Modification

To resolve the warning I'm given the following options.

  • Add default case
  • Add missing case statements
  • Add @SuppressWarnings 'incomplete-switch' to foo()

If I add the missing case statement then the warning no longer appears. The same as adding the missing case makes your error warning disappear from intellij.

Without the statement for case NONE you can only see two cases, both of which return true. Without knowing the structure of Modification and the extra value of NONE it looks like this loop would just return true on the first iteration of the loop.

Of course the compiler should actually know that there are more values for Modification than SET and REMOVE so the warning is just for good style. Basically your code works but here's how to improve it.

I would choose to add a default statement rather than the missing case. This would be more future proof in case more values are later added to the enum. E.G.

switch (modification) 
{
  case SET:
  case REMOVE:
    return true;
  default:
    break;
}

Personally I'm not a fan of using the fall through on switch statements. What you gain in making the code concise you lose in legibility IMHO. If someone later comes and adds a case between SET and REMOVE it could introduce a bug. Also, having a return statement mid-way through a method can also cause problems. If someone wants to add some code just before the return they may miss all the places. If the method is very simple then multiple returns is fine but you've stated that this is a simplified example and so if this block of code is complicated I would avoid it.

If you're able to use Java 8 then this looks to be the perfect use case for the new stream API. Something like the following should work.

return sList.stream().anyMatch(
  modification -> (modification==Modification.SET || modification==Modification.REMOVE)
);

Upvotes: 9

Lars de Bruijn
Lars de Bruijn

Reputation: 1518

Your switch case always breaks or returns. In the first case, you do nothing aka it falls through. The second case returns which causes both the switch and the loop to stop. In the third case you break the switch statement which causes it to stop. It does not however stop the for loop (aka, it keeps iterating).

Either add specific functionality for the SET case or change your behaviour on the REMOVE and NONE cases.

public enum Modification {
    NONE, SET, REMOVE;
}

boolean foo(){
    for (S s : sList) {
        final Modification modification = s.getModification();
        switch (modification) {
            case SET:
               // This case falls through to the REMOVE case
            case REMOVE:
                return true; // This statement stops the switch, loop and returns true
            case NONE:
                break; // This statement stops the switch and continues the loop.
        }
    }
    return false;
}

Your switch is not looping without the NONE case because return breaks the loop and returns a value from the function. break breaks the switch loop but continues the for loop.

By request of OP an extra explanation.

Falling through means the next case will be executed until a stop (break or return) is reached. This makes the following code snippets equivelant:

case SET:
case REMOVE:
    return true;

is the same as:

case SET:
    return true;
case REMOVE:
    return true;

Upvotes: 0

AngryDuck
AngryDuck

Reputation: 4607

i assume these are your only three cases right?, so basically its saying you are going to hit one of the first two and instantly return true, therefore not looping, just add a default case and everything should work ok, this is good practice also btw.

basically it cant see a case where it doesnt just return instantly without iterating the loop

Upvotes: 2

Related Questions