🔒 Locked

How can I improve this code? (C++ enums)

Started by Storyyeller Oct 3, 2011 at 4:47 PM 21 replies 3.9k views

Original Post

Oct 3, 2011 at 4:47 PM

So in my game I have various secret items, each associated with a name and image. The problem is coming up with a way to automatically assign an index to each one (from 0 to n-1) so that they can be iterated over and used as indicies in a bitmap.

Enums solve the automatic index assignment problem, but they require type casting to iterate over or use as indices. So what should I do? Here's my current implementation. The problem is that I would have to manually change all the values when adding or removing a secret.

[source lang='cpp']typedef unsigned int Secret;

namespace Secrets
{
Secret begin();
Secret end();

Secret POTATO();
Secret NONPOISONOUSMUSHROOM();
Secret TM49();
Secret OBVIOUSSECRET();
Secret DELICIOUSCAKE();
Secret BIGDIPPER();

std::string Name(Secret s);
ImgHandle Image(Secret s);
}[/source]

[source lang='cpp']namespace Secrets
{
const Secret ePOTATO = 0;
const Secret eMUSHROOM = 1;
const Secret eTM49 = 2;
const Secret eOBVIOUS = 3;
const Secret eCAKE = 4;
const Secret eDIPPER = 5;
const Secret eNUMSECRETS = 6;

Secret begin() {return 0;}
Secret end() {return eNUMSECRETS;}

Secret POTATO() {return Secret(ePOTATO);}
Secret NONPOISONOUSMUSHROOM() {return Secret(eMUSHROOM);}
Secret TM49() {return Secret(eTM49);}
Secret OBVIOUSSECRET() {return Secret(eOBVIOUS);}
Secret DELICIOUSCAKE() {return Secret(eCAKE);}
Secret BIGDIPPER() {return Secret(eDIPPER);}

std::string Name(Secret s)
{
switch (s)
{
case ePOTATO: return "Potato Battery";
case eMUSHROOM: return "Nonpoisonous Mushroom";
case eTM49: return "TM49 (Tri Attack)";
case eOBVIOUS: return "Very Obvious Secret";
case eCAKE: return "Delicious Cake";
case eDIPPER: return "Big Dipper";
default: assert(false); return "INVALID SECRET";
}
}

ImgHandle Image(Secret s)
{
switch (s)
{
case ePOTATO: return ImgHandle(ImgDataStruct("Secrets/potato.png",0,99,0));
case eMUSHROOM: return ImgHandle(ImgDataStruct("Secrets/nonpoisonousmushroom.bmp",255,0,255));
case eTM49: return ImgHandle(ImgDataStruct("Secrets/tm49.bmp",0,255,0));
case eOBVIOUS: return ImgHandle(ImgDataStruct("Secrets/obvioussecret.bmp",0,255,0));
case eCAKE: return ImgHandle(ImgDataStruct("Secrets/deliciouscake.bmp",0,255,0));
case eDIPPER: return ImgHandle(ImgDataStruct("Secrets/bigdipper.bmp",0,255,0));
default: assert(false); return ImgHandle();
}
}
}[/source]

Dr Yap

Oct 3, 2011 at 4:54 PM

I the compiler you are using supports the C++11 standard then you can use an enum and set the underlying type of that enum so no casting will be necessary.

fastcall22

Oct 3, 2011 at 4:55 PM

What about an array of Secret objects?


 struct Secret {
 enum Index {
 mushroom,
 cake,
 dipper,
 /* ... */
 count
 };
 
 Index idx;
 string name;
 ImgHandle image;
 };
 
 Secret theSecrets[Secret::count] = {
 { Secret::mushroom, "Nonpoisonous Mushroom", ImgHandle( ... ) },
 { Secret::cake, "Delicious Cake", ImgHandle( ... ) }
 /* ... */
 };

First, it seems to make sense to make some sort of object, since all Secrets seem to have an ID, a description, and an ImgHandle. From here, we can make an array for the type of all the secret-types. Adding or removing a secret requires changing the Secret::Index enum, and will emit a compiler error if you forget to update the array. An array can also be iterated over, and the Index enum doesn't require type casting.

However, this approach requires that the IDs of the items in the array match up. A simple sort on program execution or an assertion could enforce this.

Another alternative would be to use an associative container: map

Dawoodoz

Oct 3, 2011 at 5:09 PM

Try to store the item types in a dynamic array and load them from a file for the ability to load a mod from a folder. Try to represent their differences using numbers and boolean flags instead of hard coded behaviours. For example: Instead of naming all the items that can be used as a weapon, store a boolean property called "is weapon" so that an item type can be removed or duplicated without much effort. If the weapon have properties for the type of bullet used, refer to another list of ammunition types.

The same problems are common when designing databases without redundancy.
http://en.wikipedia....e_normalization

Storyyeller

Oct 3, 2011 at 6:03 PM

I tried the array method and it seems to work well. Thanks!

yckx

Oct 3, 2011 at 6:14 PM

If you're indexing by enum then the std::map is the more appropriate solution.

Bregma

Oct 3, 2011 at 7:27 PM

If you're indexing by enum then the std::map is the more appropriate solution.

This.
[source]
#include
#include
#include

enum Secret
{
POTATO,
MUSHROOM,
TM49,
OBVIOUS,
CAKE,
DIPPER,
NUMSECRETS,
};

int
main(int argc, char* argv)
{
std::map mapping = {
{ POTATO, "Potato Battery" },
{ MUSHROOM, "Nonpoisonous Mushroom" },
{ TM49, "TM49 (Tri Attack)" },
{ OBVIOUS, "Very Obvious Secret" },
{ DIPPER, "Big Dipper" },
};

for (auto p = mapping.begin(); p != mapping.end(); ++p)
{
std::cerr << p->first << ": " << p->second << "\n";
}
}
[/source]
Compiled and tested on Ubuntu 10.04 with g++ 4.5.2 (this is an older compiler does not support the current C++ standard, so no lambdas or [font="Courier New"]for (auto p: mapping)[/font] range iteration).

If you replace the [font="Courier New"]std::string[/font] as the [font="Courier New"]map[/font] value with a struct, you can get both the name and image.

Storyyeller

Oct 3, 2011 at 8:18 PM

A map seems like overkill when the keys are just consecutive integers.

yckx

Oct 3, 2011 at 9:44 PM

It's an associative container, which is what should be used when you're not indexing by integer. Otherwise I'd recommend a std::vector. An array should typically be avoided if possible. It's a C holdover; using the STL is the proper way to approach containers in C++.

Slavik81

Oct 4, 2011 at 12:43 AM

If you're indexing by enum then the std::map is the more appropriate solution.

It seems a bit odd to use a binary tree when an vector/array is basically a perfect hash for enums. Just don't encode meanings into the integer representation of your enums and there's no problem.

The C++ practice of giving enums special integer values is kind of weird. It almost feels like enums are a hack that paper over a hole in the language. Particularly when they're used as flags.

alvaro

Oct 4, 2011 at 1:46 AM

I like fastcall22's solution, except I wouldn't store the index as part of the object. Replacing a simple array with an std::map is a pessimization that I can't understand.

Storyyeller

Oct 4, 2011 at 1:49 AM

Yeah, that's what I ended up doing. There's no real reason to store a copy of the index.

[source lang='cpp']typedef unsigned int Secret;

namespace Secrets
{
//Order must be kept in sync with array in secret.cpp
enum SecretID : unsigned int {POTATO, NONPOISONOUSMUSHROOM, TM49, OBVIOUSSECRET, DELICIOUSCAKE, BIGDIPPER, NUMSECRETS};

inline Secret begin() {return 0u;}
inline Secret end() {return NUMSECRETS;}

std::string Name(Secret s);
ImgHandle Image(Secret s);
}[/source]
[source lang='cpp']
namespace Secrets
{
struct SecretData{
std::string name;
ImgDataStruct imgdata; //Don't convert to handle until runtime
};

const SecretData data[NUMSECRETS] = {
{"Potato Battery", ImgDataStruct("Secrets/potato.png",0,99,0)},
{"Nonpoisonous Mushroom", ImgDataStruct("Secrets/nonpoisonousmushroom.bmp",255,0,255)},
{"TM49 (Tri Attack)", ImgDataStruct("Secrets/tm49.bmp",0,255,0)},
{"Very Obvious Secret", ImgDataStruct("Secrets/obvioussecret.bmp",0,255,0)},
{"Delicious Cake", ImgDataStruct("Secrets/deliciouscake.bmp",0,255,0)},
{"Big Dipper", ImgDataStruct("Secrets/bigdipper.bmp",0,255,0)}
};

std::string Name(Secret s)
{
assert(s < NUMSECRETS);
return data[s].name;
}

ImgHandle Image(Secret s)
{
assert(s < NUMSECRETS);
return data[s].imgdata;
}
}[/source]

Cornstalks

Oct 4, 2011 at 1:51 AM

I like fastcall22's solution, except I wouldn't store the index as part of the object. Replacing a simple array with an std::map is a pessimization that I can't understand.

Storyyeller

Oct 4, 2011 at 1:55 AM

If enum values are really undefined, then there are bigger issues and using a different collection won't fix that.

The only case I can imagine where a map would be useful is if the enum values were discontinuous and had a large range, but that is specifically not the case here.

Wooh

Oct 4, 2011 at 2:21 AM

I agree that a map is overkill here. Using enums to index an array is perfectly fine.

If not specified the first enumerator has value zero. For the rest, if not specified the value of the enumerator is one more than the previous enumerator.

Slavik81

Oct 4, 2011 at 2:28 AM

I think its mostly because any enum isn't guaranteed to be zero initialized (I'm sure there's a more technically correct term) or contiguous. While there isn't anything inherently wrong with fastcall's solution, the more general solution is something like a map.

In C++, enums are guaranteed to start at zero and count up one by one from there.

Without that guarantee, a lot of normal enum usages would break down.

enum PizzaToppings
 {
 Cheese, Pepperoni, Anchovies, ToppingsCount
 };

ToppingsCount will be 3.

alvaro

Oct 4, 2011 at 2:28 AM

[quote name='alvaro' timestamp='1317692769' post='4868786']
I like fastcall22's solution, except I wouldn't store the index as part of the object. Replacing a simple array with an std::map is a pessimization that I can't understand.

I think its mostly because any enum isn't guaranteed to be zero initialized (I'm sure there's a more technically correct term) or contiguous. While there isn't anything inherently wrong with fastcall's solution, the more general solution is something like a map.
[/quote]

In C++ an enum is guaranteed to start at 0 and continue with 1, 2, 3, etc.

From the ANSI C++ standard (7.2 - Enumeration declarations [dcl.enum]):

If the first enumerator has no initializer, the value of the corresponding constant is zero. An enumerator-definition without an initializer gives the enumerator the value obtained by increasing the value of the previous enumerator by one.[/quote]

Cornstalks

Oct 4, 2011 at 2:52 AM

I understand that. That's why I said there was nothing wrong with fastcall's code. I said you aren't guaranteed with any enum, meaning there exists at least one enum where it does not start at zero (i.e. if he set mushroom to 1000). Perhaps I could have chosen clearer words, because I can see now how you may have thought I meant you aren't guaranteed that with any enum at all. I was specifically referring to cases where the first enumerator has a non-zero initializer.

This is safe and well defined in this case. But there may come a time when you're using some external library, and you cant always assume the index will start at zero, because the library's author may have specified a different initialization. I just wanted the OP to be aware of that and know that if he is going to use an enum to index an array, he needs to know exactly how the enum was defined, or else he may end up with a big mess. Plus I was just responding to alvaro's comment, saying that there are indeed times when a map is both preferred and necessary.

Like I said, it is for this reason (and the fact that the programmer may have chosen non contiguous values) that maps are considered the "more general" solution. It does not, however, invalidate fastcall's solution in this case.

Storyyeller

Oct 4, 2011 at 3:18 AM

I'm the one defining the enum. It's right there in the code. Of course I know what values it can take.

Cornstalks

Oct 4, 2011 at 3:28 AM

I know you do. I never said you didn't. But I don't know personally, and I don't know whether or not you would make this kind of assumption in the future, so I thought I would just add that and make it explicitly clear, just in case.

If you already do know everything I have stated, wonderful. Then I hope what I said will help someone who may browse these forums in the future and hopefully help them avoid making a poor assumption by helping them understand *why* its a poor assumption.

In this particular case, its safe, like I've said before. But its not safe in every possible case. You may already know that, but I have no way of knowing whether you do or not, nor do I know if any potential beginners browsing this thread know that or not. So I tried to add a small, clear comment as to why it isn't safe in every case.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.