tags:

views:

3324

answers:

8

I have this enum:

enum ButtonState {
    BUTTON_NORMAL = 0,
    BUTTON_PRESSED = 1,
    BUTTON_CLICKED = 2
};

const u8 NUM_BUTTON_STATES = 3;

In my Button class I have member variables ButtonState state; and ButtonColors colors[NUM_BUTTON_STATES];. When drawing the button, I use colors[state] to get the colours for whatever state the button is in.

My questions:

  1. Is this good programming style? Is there a better way to do it? (I usually only use enums with switch statements... using an enum as an array index doesn't feel right.)
  2. Do I have to specify the values of the enum? It seems to start from 0 by default and increment by 1 but is it guaranteed to work that way in all compilers?
+1  A: 

Question 1: I think it's good programming style. I use it all the time. Question 2: As far as I know, it is guaranteed to work that way, so you don't have to specify the values.

And I would put NUM_BUTTON_STATES into the enum as well.

Very good idea with putting NUM_BUTTON_STATES in the enum!
yjerem
+8  A: 

Using an enum is ok. But you don't have to specify values for every item. It's enough to specify the first value. I wouldn't assume that enums start at 0, because I've used compilers which used 1 as the starting value (not for PCs but some compilers for microcontrollers have some weird behavior). Also, you could get rid of the const:

enum ButtonState {
    BUTTON_NORMAL = 0,
    BUTTON_PRESSED,
    BUTTON_CLICKED,
    NUM_BUTTON_STATES
};
Stefan
+5  A: 

Is this good programming style?

I think so. I do the same thing quite frequently.

Is there a better way to do it?

class Button
{
public:
    // Used for array indexes!  Don't change the numbers!
  enum State {
    NORMAL = 0,
    PRESSED,
    CLICKED,
    NUMBER_OF_BUTTON_STATES
  };
};

Drawback is that NUMBER_OF_BUTTON_STATES is now a valid Button::State value. Not a big issue if you are passing these values around as ints. But trouble if you are actually expecting a Button::State.

Using an enum as an array index doesn't feel right.

It's fine. Just DOCUMENT it, so the next guy knows what's going on! (That's what comments are for.)

Do I have to specify the values of the enum?

With no '=' assignment, enum's are supposed to start at zero and increment upwards.

If a enum entry has an '=' assigned value, subsequent non '=' enum entries continue counting from there.

Source: The Annotated C++ Reference Manual, pg 113

That said, I like to specify the initial value just to make the code that much clearer.

Mr.Ree
You should use a namespace instead of an empty class. It matches your intent more accurately.
Tom
The C and C++ language standards specify that the first enumeration constant in an enum has value zero, unless you assign it a different value. Any compiler that doesn't do this is obviously noncompliant.
ChrisN
ChrisN is right, but I have worked with such compilers, sadly -- and within the last 10 years, no less. If you have any concerns about cross-compiler compatibility, specify the zero.
Patrick Johnmeyer
Class would have members ButtonState and ButtonColors, as per OP. I left them off to avoid distracting from the main point.
Mr.Ree
@Patrick Johnmeyer, can you name that particular compiler?
Alex B
It was at a previous job in which I was using three different compilers for different targets, so I might be misremembering -- but I believe it was a particular version of the Green Hills C++ compiler. The first value could be anything if not set; it was basically as if the compiler was using an uninitialized variable for the first value.
Patrick Johnmeyer
+1  A: 

It is perfectly normal to use an enum for indexing into an array.

You don't have to specify each enum value, they will increment automatically by 1. Letting the compiler pick the values reduces the possibility of mistyping and creating a bug, but it deprives you of seeing the values, which might be useful in debugging.

Mark Ransom
+2  A: 

Style-wise, it's just fine.

Pascal-based languages like Delphi allow array bounds to be specified as an enum type, so you can only use items of that specific type as an index.

Roddy
Thank's for pointing that out.
Martin
+4  A: 

Yeah it will work well. That said, in any case, you really should put another entry in your enumeration defining the value of the amount of items:

enum ButtonState {
    BUTTON_NORMAL,
    BUTTON_PRESSED,
    BUTTON_CLICKED,
    STATE_COUNT
};

Then you can define the array like

Color colors[STATE_COUNT];

otherwise, it's a mess to keep the amount of states synchronous with the size of the array. Enumerations will always start with zero if not otherwise initialized, and then each additional entry will be assigned a value one above the previous one, if not otherwise initialized. Of course it also wouldn't hurt if you put a zero explicitly if you want. If you don't mind additional code, i would wrap the access to the raw array using a function like

Color & operator[](ButtonState state) {
    return array[state];
}

Or an equivalent getColor function forwarding the request. That would forbid directly indexing the array with some integer, which would almost certainly at some point fail because one gets the indexes wrong.

Johannes Schaub - litb
+1  A: 

It's fine, but I'd want to do some bounds checking on the array, as if someone adds another ButtonState, you'll have a problem.

Also, the elements of the colors array are immutable, so maybe look at using a different collection to array so that you can enforce that immutability. Maybe a Dictionary<ButtonState,ButtonColor>

WOPR
A: 

Just use enum.ordinal() to get the index.

Reference: http://forums.sun.com/thread.jspa?threadID=5366582

Prasanna