refactoring multiple if-else conditionals in a method

Question

I am in the process of refactoring my existing code. It actually works fine, but it is a bit cluttered with multiple if-else conditionals checking the value of one variable and change the value of a second variable to an updated value taken from a fixed enumeration structure.

else if (var1 == 'valueX')
{
    if (var2 == MyEnum.A)
        var2 = MyEnum.B;
    else if (var2 == MyEnum.B)
        var2 = MyEnum.C;
    else if (var2 == MyEnum.C)
        var2 = MyEnum.D;
    else if (var2 == MyEnum.D)
        var2 = MyEnum.A;
}

else if (....)
{
..similar block of conditionals
}

I am a bit confused as to what is the best way to refactor and clean-up this code. Would you suggest the use of a switch perhaps? Or something more elegant?

Thanks in advance!

Answer 1

At least with J2SE 1.5 forward, you can give enums extra attributes. This means you might be able to replace that entire string of if-else with something that looks like

var2 = var1.getNextInSequence();

Now, in this case, it looks like you would want the attribute to be a reference to another enum, which adds some wrinkles, for example you can't forward reference enums when you initialize them, but there might be a workable solution for you this way.

When the attributes aren't other instances of the same enum, this kind of thing will work:

public enum Animal {
    FOX(4),
    CHICKEN(2),
    WORM(0);

    private int countLegs;

    Animal(int n) {
        countLegs = n;
    }

    public int getLegCount() {
        return countLegs;
    }
    // .. more getters setters etc
}

But when the enum is self-referential, you have to be careful about the order of declaration of your instances. I.e., this will have some issues:

public enum Animal {
    FOX(4, CHICKEN),    // 'CHICKEN' doesn't exist yet
    WORM(0, null),
    CHICKEN(2, WORM);    // this actually will compile

    private int countLegs;
    private Animal eatsWhat;

    Animal(int n, Animal dinner) {
        countLegs = n;
        eatsWhat = dinner;
    }

    public int getLegCount() {
        return countLegs;
    }
    // .. getters, setters, etc
}

So if you had need of a circular set of references among the enums, you'd have to work something else out, but if not, you could use this technique, though you may have to order your enum instances just so to make it work.

Answer 2

The classic answer to refactoring conditionals is Replace Conditional With Polymorphism. In this case, if each of MyEnum knew what its successor was, you could simply say (in the 'valuex' case: var2 = var2.successor. For var1 - if it could be an object that implemented an interface that knew how to handle whatever you're doing inside the loop, and each implementing class knew what it, specifically, should do... Well, you'd be done.

Update:

And here's a dandy little successor function in a test case:

public class EnumTest extends TestCase {
    private enum X {
        A, B, C;
        public X successor() {
            return values()[(ordinal() + 1) % values().length];
        }
    };

    public void testSuccessor() throws Exception {
        assertEquals(X.B, X.A.successor());
        assertEquals(X.C, X.B.successor());
        assertEquals(X.A, X.C.successor());
    }
}

Answer 3

You can use a simple map:

enum MyEnum { A, B, C };

Map<MyEnum, MyEnum> VALUE_X = new HashMap<MyEnum, MyEnum>() {{
    put(MyEnum.A, MyEnum.B);
    put(MyEnum.B, MyEnum.C);
    ...
}};

// define another kind of ordering
Map<MyEnum, MyEnum> VALUE_Y = new HashMap<MyEnum, MyEnum>() {{
    put(MyEnum.A, MyEnum.D);
    put(MyEnum.B, MyEnum.A);
    ...
}};

This way, the logic of the next var2 value isn't hard-coded in the enum itself, and can be dependant of context (i.e. value of var1):

if ("valueX".equals(var1)) {  // use equals() instead of == for Strings
    var2 = VALUE_X.get(var2);
}
else if ("valueY".equals(var1)) {
    var2 = VALUE_Y.get(var2);
}