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

Started by
21 comments, last by Storyyeller 12 years, 6 months ago
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]
I trust exceptions about as far as I can throw them.
Advertisement
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.
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<Secret::Index, Secret>
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
I tried the array method and it seems to work well. Thanks!
I trust exceptions about as far as I can throw them.
If you're indexing by enum then the std::map is the more appropriate solution.

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

This.
[source]
#include <iostream>
#include <map>
#include <string>


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


int
main(int argc, char* argv)
{
std::map<Secret, std::string> 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.

Stephen M. Webb
Professional Free Software Developer

A map seems like overkill when the keys are just consecutive integers.
I trust exceptions about as far as I can throw them.
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++.

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.

This topic is closed to new replies.

Advertisement