An enum that has situationally invalid entries

Question

Is it considered bad style to have that BOTH value in the Direction enum? Problems can arise from BOTH being present, for example when trying to build a procedural UI for the Car class, it (the generator) would see the setTurnSignal(Direction) method and create a way for the user to give it a Direction, but it would suggest all values, of which one is illegal.

Conversely, not having BOTH causes issue with the getBlinkerStatus() as it is no longer able to indicate that both are on. There are a couple ways I could address that issue, such as creating more blinker getters, but that or switching form an enum to bit based values and building masks for them.

enum Direction{
    LEFT, RIGHT, BOTH, NONE;
}

class Car{
    Direction turnSignal;
    boolean hazards; 

    void setTurnSignal(Direction direction){
        if(Direction.BOTH.equals(direction)){
            throw new IllegalArgumentException("Turn signal cannot be set to the given direction: " + direction);
        }else{
            turnSignal = direction;
        }
    }

    Direction getBlinkerStatus(){
        if(hazards){
            return Direction.BOTH;
        }else{
            return turnSignal;
        }
    }

Answer 1

There are a couple ways I could address that issue, such as creating more blinker getters, but that or switching form an enum to bit based values and building masks for them.

java.util.EnumSet<E> is the piece you are looking for here.

public class Car {
    static enum Direction {
        LEFT, RIGHT
    }

    EnumSet<Direction> turnSignal;

    void setTurnSignal(Direction direction) {
        turnSignal = EnumSet.of(direction);
    }

    void setHazards() {
        turnSignal = EnumSet.of(Direction.LEFT,Direction.RIGHT);
    }

    EnumSet<Direction> getBlinkerStatus() {
        return turnSignal;
    }
}

That said, this probably isn't the right approach. If you are trying to capture the state of the blinkers, then you should probably be implementing a state machine that tracks what is going on. A state machine is absolutely what you want if you are worried about the hazards being enabled and disabled when the turn signal is going (as you indicate in your comment).

The straight forward model would be that you have six states - [LEFT,NONE,RIGHT] x [HAZARDS ON, HAZARDS OFF] - with simple transitions between them.

You can hand roll a state machine easily enough, but beware -- it's common for state machines to snowball in complexity, as you learn more about the system you are modeling. So it might be worth investing in a library like stateless4J, which gives you a DSL for wiring together your state transition diagram.

public class FSM {
    enum State {
        OFF, LEFT, RIGHT, HAZARDS
        // Unimaginitive names for the "hidden" states.
        , HAZARDS_LEFT, HAZARDS_RIGHT
    }

    enum Toggle {
        LEFT, RIGHT, HAZARDS
    }

    static StateMachine<State, Toggle> create() {
        StateMachine<State, Toggle> fsm = new StateMachine<State, Toggle>(State.OFF);
        fsm.configure(State.OFF)
                .permit(Toggle.LEFT, State.LEFT)
                .permit(Toggle.RIGHT, State.RIGHT)
                .permit(Toggle.HAZARDS, State.HAZARDS);
        fsm.configure(State.HAZARDS)
                .permit(Toggle.LEFT, State.HAZARDS_LEFT)
                .permit(Toggle.RIGHT, State.HAZARDS_RIGHT)
                .permit(Toggle.HAZARDS, State.OFF);
        // etc.

        return fsm;
    }
}

Alternatively, you might consider a state machine design where some of these states are substates of others; stateless4j has substate support, but I haven't explored it enough to have an opinion on whether this should apply for your problem.

Answer 2

You might not be aware of it, but there is another data structure you can use here. You mention bit-based values in your question, but there's no need for you to handle that logic directly.

There is a EnumSet data-type which might be useful here,

You could reduce your Direction enum to simply LEFT and RIGHT. For the BOTH and NONE, you can use the EnumSet.

enum Direction {
    LEFT, RIGHT;
}

EnumSet<Direction> directions = EnumSet.noneOf(Direction.class);

The EnumSet works just like an ordinary Set, so you can add and remove LEFT and RIGHT from it.

From the EnumSet documentation:

Enum sets are represented internally as bit vectors. This representation is extremely compact and efficient. The space and time performance of this class should be good enough to allow its use as a high-quality, typesafe alternative to traditional int-based "bit flags."

Answer 3

The problem here is that your class is not symmetrical.

If you gave a 'getter' that can return Direction.BOTH (when hazards == true), then you should have a matching 'setter'.

In your class, I imagine you have a setHazards, as well as the method setTurnSignal(Direction). You should replace those methods with a simple setDirection(Direction), and remove the hazards boolean.

This would make your API symmetrical, and the logic clearer:

enum Direction{
    LEFT, RIGHT, BOTH, NONE;
}

class Car{
    Direction turnSignal = Direction.NONE;

    void setBlinkerStatus(Direction direction){
        turnSignal = direction;
    }

    Direction getBlinkerStatus(){
        return turnSignal;
    }
}

Answer 4

I believe you are confounding two concepts. The first concept is that of a turn signal, which is in one of three states: left-blinking, right-blinking or neither. The second concept is the state of the signalling lamps: left-blinking, right-blinking, both or neither.

Because the physical devices have overloaded functionality (turn signalling and emergency signalling), you will have to overload something; either your enumeration or your class responsibilities.

I don't agree with @rolfl that you should remove the functionality of turn signals and hazard blinkers. Instead, add functionality at that level:

void setSignalLeft() { setBlinkerStatus(LEFT); }
void setSignalRight() { setBlinkerStatus(RIGHT); }
void setSignalOff() { setBlinkerStatus(OFF); }

void setHazardOn() { setBlinkerStatus(BOTH); }
void setHazardOff() { setBlinkerStatus(OFF); }
boolean isHazard() { return getBlinkerStatus() == BOTH; }

You may also want to add interlocks to prevent impossible operations.