Sign in to follow this  
Storyyeller

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

Recommended Posts

Storyyeller    215
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]

Share this post


Link to post
Share on other sites
Dr Yap    112
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.

Share this post


Link to post
Share on other sites
fastcall22    10840
What about an array of Secret objects?

[code]
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( ... ) }
/* ... */
};
[/code]

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>

Share this post


Link to post
Share on other sites
Dawoodoz    461
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.
[url="http://en.wikipedia.org/wiki/Database_normalization"]http://en.wikipedia....e_normalization[/url]

Share this post


Link to post
Share on other sites
Bregma    9202
[quote name='yckx' timestamp='1317665640' post='4868639']
If you're indexing by enum then the std::map is the more appropriate solution.
[/quote]
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.

Share this post


Link to post
Share on other sites
yckx    1298
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++.

Share this post


Link to post
Share on other sites
Slavik81    360
[quote name='yckx' timestamp='1317665640' post='4868639']
If you're indexing by enum then the std::map is the more appropriate solution.
[/quote]
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.

Share this post


Link to post
Share on other sites
alvaro    21246
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.

Share this post


Link to post
Share on other sites
Storyyeller    215
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]

Share this post


Link to post
Share on other sites
Cornstalks    7030
[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.
[/quote]
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.

Share this post


Link to post
Share on other sites
Storyyeller    215
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.

Share this post


Link to post
Share on other sites
Wooh    1088
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.

Share this post


Link to post
Share on other sites
Slavik81    360
[quote name='Cornstalks' timestamp='1317693081' post='4868790']
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++, 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.

[code]enum PizzaToppings
{
Cheese, Pepperoni, Anchovies, ToppingsCount
};
[/code]

ToppingsCount will be 3.

Share this post


Link to post
Share on other sites
alvaro    21246
[quote name='Cornstalks' timestamp='1317693081' post='4868790']
[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.
[/quote]
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]):

[quote]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]

Share this post


Link to post
Share on other sites
Cornstalks    7030
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.

Share this post


Link to post
Share on other sites
Cornstalks    7030
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.

Share this post


Link to post
Share on other sites
yckx    1298
Cornstalks has done a good job explaining my reasoning, which I failed to add in my post because I was typing it on my phone. fastcall22's solution works, and that's fine. Run with it.

But not every enumeration is guaranteed to start from 0 or be contiguous. And just because this one is typical doesn't mean it will necessarily remain so for all time (although it may). There have been plenty of times when I made some apparently innocuous changes to code and end up breaking other bits of code here and there that assume the implementation of the bit of code that was originally changed. Hence my recommendation. I'm okay with not everyone agreeing with me ;)

Basic arrays are a part of the language, so I'm not going to say never use them. But I prefer an appropriate stl container wherever possible.

Share this post


Link to post
Share on other sites
Storyyeller    215
It seems to me like a data driven approach works best when you have a well defined engine component and logic/data component that are planned out ahead of time and a fixed interface between the two. Whereas here, I've got a sprawling multigenre project with no real planning. Actually the entire game is hardcoded, though recently I've experimented with writing scripts to generate the code associated with level data.

One thing I could do is use forward declaration of enums to reduce recompilation, but I'm still using gcc 4.4.1 as I'm a windows noob and don't really know how to mess around with installing other versions of mingw.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this