63

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?
Shog9
  • 156,901
  • 35
  • 231
  • 235
Paige Ruten
  • 172,675
  • 36
  • 177
  • 197

8 Answers8

53

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
  • 8,320
  • 27
  • 30
  • 2
    You should use a namespace instead of an empty class. It matches your intent more accurately. – Tom Dec 31 '08 at 23:30
  • 3
    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 Jan 01 '09 at 11:47
  • 2
    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 Jan 01 '09 at 13:50
  • Class would have members ButtonState and ButtonColors, as per OP. I left them off to avoid distracting from the main point. – Mr.Ree Jan 11 '09 at 05:18
  • 2
    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 Mar 29 '10 at 13:38
26

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
  • 496,577
  • 130
  • 894
  • 1,212
15

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
  • 43,293
  • 10
  • 75
  • 117
  • 5
    You're effectively saying 'You shouldn't assume compilers do what the Standard requires them to do', which isn't fundamentally bad advice. The Standard requires `enum`s to start at 0 unless otherwise specified, and programmers shouldn't shy from taking advantage of that - or finding a competent compiler. – underscore_d Jan 30 '16 at 08:08
5

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
  • 66,617
  • 42
  • 165
  • 277
3

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.

1

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
  • 299,747
  • 42
  • 398
  • 622
  • **enums can't be used for indexing in cpp if they are scoped ones (like enum structs or class)**. With scoped enums we need to use explicit casting. Read more here: [link](https://en.cppreference.com/w/cpp/language/enum) – Shashank Jan 25 '22 at 10:29
  • @Shashank I don't think scoped enums existed when I wrote this answer, looks like they were added in C++11. – Mark Ransom Jan 25 '22 at 14:24
1

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
  • 5,313
  • 6
  • 47
  • 63
  • Who said arrays can't be immutable? And C++ has no standard container called `Dictionary`; maybe you meant `map`. Still, I don't see the point of that for most cases with simple and consecutive numeric 'keys'; at best, it avoids the user having to enforce/remember the index order, but it does so at a trade-off of lookup speed. – underscore_d Jan 30 '16 at 08:12
0

One of the ways is to create an internal vector inside your class and override operator []: (here is the link to online code)

#include <iostream>
#include <vector>

template<typename DATA_TYPE, typename INDEX_TYPE>
class MyVector
{
protected:
    std::vector<DATA_TYPE> pp;

public:    
    template<typename ... ARGS>
    MyVector(ARGS ... args): pp{args...} {};

    DATA_TYPE& operator[](INDEX_TYPE x) { 
        return pp[static_cast<size_t>(x)];
    };

    auto size() const { return pp.size(); }

    // implement other menthods


};

enum class EWeekDays{
    SUN, MON, TUE, WED, THU, FRI, SAT
};


int main()
{
    MyVector<int, EWeekDays> q{2,3,4,5};

    std::cout << q[EWeekDays::MON] << "\n";

    return 0;
}