Multiple if statements having incorrect output

Question

I want to fill the acts array with values of some enum. While iterating I want to input commands from console, but my if statements don't find any match and I always get the output "Incorrect".

My code:

Action[] acts = new Action[n];
for(int i = 0; i < n; i++) {
    System.out.println("Enter act: ");
    Scanner in1 = new Scanner(System.in);
    String s = in1.next();
    acts[i] = new Action();
    if (s.equals("rotate_forw")) 
        acts[i].type = ActionType.RotF;
    if (s.equals("rotate_back")) 
        acts[i].type = ActionType.RotB;
    if (s.equals("shift_forw")) 
        acts[i].type = ActionType.ShiftF;
    if (s.equals("shift_back")) 
        acts[i].type = ActionType.ShiftB;
    else 
        System.out.println("Incorrect");
}

You described what you want, but not what problem you are facing. Do you get any error/exception/incorrect result? — Pshemo, Nov 27 '16 at 07:08

Eran · Accepted Answer · 2017-08-08T17:36:27.010

Your else clause applies only to the last if statement, so you get the "Incorrect" output whenever s.equals("shift_back") is false.

Your statements should be replaced with a single if-else-if...-else statement, so that "Incorrect" is only printed if all the conditions are false :

Action[] acts = new Action[n];
for(int i = 0; i < n; i++) {
    if (s.equals("rotate_forw"))
        acts[i].type = ActionType.RotF;
    else if (s.equals("rotate_back"))
        acts[i].type = ActionType.RotB;
    else if (s.equals("shift_forw"))
        acts[i].type = ActionType.ShiftF;
    else if (s.equals("shift_back"))
        acts[i].type = ActionType.ShiftB;
    else
        System.out.println("Incorrect");
}

You should also consider what you want to assign to acts[i].type when the input is incorrect. Perhaps you should throw an exception in this case.

score 1 · Answer 2 · edited May 23 '17 at 12:08

While @Eran's answer is correct, I'd like to suggest a different approach that encapsulates the enum with the translation from the external coding. Consider this:

public class EnumDemo
{
    public static enum ActionType
    {
        Incorrect(""),
        RotF("rotate_forw"),
        RotB("rotate_back"),
        ShiftF("shift_forw"),
        ShiftB("shift_back");

        private String code;
        private ActionType(String code)
        {
            this.code = code;
        }

        public static ActionType fromString(String code)
        {
            return Arrays.stream(ActionType.values())
                         .filter(v->v.code.equals(code))  
                         .findFirst()
                         .orElse(ActionType.Incorrect);
        }
    }

    public static void main(String[] args)
    {
        String[] testData = {  
            "rotate_forw", 
            "rotate_back", 
            "shift_forw", 
            "shift_back", 
            "junk", 
            null };

        Arrays.stream(testData)
              .forEach(t->System.out.printf("\"%s\" -> ActionType.%s\n", t, ActionType.fromString(t)));
    }
}

This uses the fact that enum constants can have associated data. I've added an instance variable code to hold the external encoding of each enum value. Then I added a static fromString(String code) method to the enum that looks up the provided code in the list of values. For 4 possibilities a simple linear search, equivalent to your if-then-else cascade, works fine. If there were dozens or more I'd set up a Map<String,ActionType> to handle the conversion.

The search using streams bears some explanation.

First create a Stream of enum values
Filter it to contain only enum values whose code matches the desired code (there should be only one)
Pick off the first entry, which comes back in a Optional. If nothing was found (i.e. the code is invalid) the Optional will be empty.
Use the orElse method to return the value if it exists or ActionType.Incorrect if not.

At first glance this might look inefficient since one expects that the filter() predicate has to scan the entire stream even if the desired element occurs early. This is a really nifty feature of Streams -- all intermediate streams are "lazy", so the filter won't iterate over the entire list if it finds the desired entry early. See this question for details.

Output:

"rotate_forw" -> ActionType.RotF
"rotate_back" -> ActionType.RotB
"shift_forw" -> ActionType.ShiftF
"shift_back" -> ActionType.ShiftB
"junk" -> ActionType.Incorrect
"null" -> ActionType.Incorrect

The last testcase shows the code is null-safe.

The biggest advantage is that the mapping is in the same place as the enum itself, so you won't have to hunt for the code when you add or remove an enum value. Also you can't forget to define the mapping since it's required by the enum's constructor.

Multiple if statements having incorrect output

2 Answers2