Is there an alternative to this long list of "if argA == argB do ..."?

Question

Edit: since attack.condition will occasionally have multiple values, a switch statement won't work!

So I have this enum which is going to grow:

enum Condition {    Null            =   0x0001,
            SelfIsUnderground   =   0x0002,
            SelfIsGround        =   0x0004,
            SelfIsAir       =   0x0008,
            SelfIsWater     =   0x0010,
            OtherIsUnderground  =   0x0020,
            OtherIsGround       =   0x0040,
            OtherIsAir      =   0x0080,
            OtherIsWater        =   0x0100,
            Tile            =   0x0200,
            CONDITION_COUNTX    =   0x03FF};

and this function which is also going to grow:

bool Attack::CanBeDone(Spirit* pSelf, Spirit* pTarget,Map* pMap)
{
    if(this->condition!=Null)
    {
        if(this->condition & SelfIsUnderground)
            if(pSelf->GetcurrentLayer()!=Underground)
                return false;

        if(this->condition & SelfIsGround)
            if(pSelf->GetcurrentLayer()!=Ground)
                return false;

        if(this->condition & SelfIsAir)
            if(pSelf->GetcurrentLayer()!=Air)
                return false;

        if(this->condition & SelfIsWater)
            if(pSelf->GetcurrentLayer()!=Water)
                return false;

        if(this->condition & OtherIsUnderground)
            if(pTarget->GetcurrentLayer()!=Underground)
                return false;

        if(this->condition & OtherIsGround)
            if(pTarget->GetcurrentLayer()!=Ground)
                return false;

        ...

Is there an alternative to writing over and over again:

    if(this->condition & arg)
        if(pSelf->GetcurrentLayer()!=value)
            return false;

?

Bonus: Will it work if I give Condition::Null the value 0x0000, SelfIsUnderground 0x0001, SelfIsGround 0x0002 and go with powers of 2 again? Eventually, Tile would end up with the value 0x0100.

Can't you loop through the `enum` values? See this: http://stackoverflow.com/questions/261963/c-iterate-through-an-enum — Maria Ines Parnisari, Apr 03 '13 at 01:38
Is a `switch` statement the kind of thing you're looking for? — drewmm, Apr 03 '13 at 01:40
@l19 That only works in that case because unnumbered enum values increment, here the OP has given them explicit bitmap values. — loganfsmyth, Apr 03 '13 at 01:41
How do I do a switch statement considering the fact that condition may and will occasionally have more than one value? Edit: As loganfsmyth points out, I can't. — Mickael Bergeron Néron, Apr 03 '13 at 01:42
@drewmm Since attack.condition will occasionally have multiple values, a switch statement won't work. — Mickael Bergeron Néron, Apr 03 '13 at 01:50
Which problem are you actually trying to solve, though? If you just want to reduce line count, you could do some clever hackery, but that would reduce your readability. If you want to do a loop over binary values, then you could have a loop with a bitshift - `for (int i = 1; i < WHATEVER; i << 1) { }`. — Infiltrator, Apr 03 '13 at 01:57
I wanted a better way than what I did but in the end I guess what I did is alright. — Mickael Bergeron Néron, Apr 03 '13 at 02:01

score 2 · Answer 1 · edited Apr 03 '13 at 03:28

First, I would write something like that:

enum Condition {    Null            =   0x0001,
            SelfBase        =   0x0002,
            SelfIsUnderground   =   SelfBase * (1 << Underground),
            SelfIsGround        =   SelfBase * (1 << Ground),
            SelfIsAir       =   SelfBase * (1 << Air),
            SelfIsWater     =   SelfBase * (1 << Water),
            SelfMask        =   SelfBase * ((1 << Max) - 1),
            OtherBase       =   0x0020,
            OtherIsUnderground  =   OtherBase * (1 << Underground),
            OtherIsGround       =   OtherBase * (1 << Ground),
            OtherIsAir      =   OtherBase * (1 << Air),
            OtherIsWater        =   OtherBase * (1 << Water),
            OtherMask       =   OtherBase * ((1 << Max) - 1),
            Tile            =   0x0200,
            CONDITION_COUNTX    =   0x03FF};

(assuming that Underground == 0 and Max == Water + 1). Then, the long list reduces to two rather crisp expressions:

if ( (SelfMask & this->condition & (SelfBase * (1 << pSelf->GetcurrentLayer()))) )
    return false;

if ( (OtherMask & this->condition & (OtherBase * (1 << pTarget->GetcurrentLayer()))) )
    return false;

return true;

This remains correct as you expand your enums. Note however that there's still some redundancy (e.g., how OtherBase and Tile are defined), which could be reduced too. Static asserts can help to make sure that the Condition enum is well-defined.

@BenVoigt: I find my version easier to understand than, say, `SelfBase << Underground`. Leave some work for the optimizer :-) — krlmlr, Apr 03 '13 at 02:03
I like what you did. It is late and I'll go to sleep but I'll look at that into details tomorow. Thanks. — Mickael Bergeron Néron, Apr 03 '13 at 02:05

Keith · Answer 2 · 2013-04-03T02:20:44.753

0

If you tie the Layer and Condition enums together, you can write something on the lines of:

enum Condition {    Null            =   0x0001,
    SelfIsUnderground   =   0x0002,
    SelfIsGround        =   0x0004,
    SelfIsAir       =   0x0008,
    SelfIsWater     =   0x0010,
    OtherIsUnderground  =   0x0020,
    OtherIsGround       =   0x0040,
    OtherIsAir      =   0x0080,
    OtherIsWater        =   0x0100,
    Tile            =   0x0200,
    CONDITION_COUNTX    =   0x03FF};

enum Layer
{
    Ground = SelfIsGround,
    Underground = SelfIsUnderground,
    Air = SelfIsAir,
    Water =  SelfIsWater
};

struct Spirit
{
    Layer GetcurrentLayer() const;

};

struct Map;

struct Attack
{
    Condition condition;
    bool CanBeDone(Spirit* pSelf, Spirit* pTarget,Map* pMap)
    {
        return (condition & pSelf->GetcurrentLayer() )&&
             ( (condition / 0x10) & pTarget->GetcurrentLayer());
    }

};

More likely, I'd expect you to define Condition in terms of Layer, but that's just details.

edited Apr 03 '13 at 02:20

answered Apr 03 '13 at 02:10

Keith

6,756
19
23

1

Can you express the hard-coded `0x10` in terms of the values defined in the enum? Otherwise you'll have to remember to update this code as you enhance the enum. – krlmlr Apr 03 '13 at 02:12
@krlmr. Well of course, in real code you'd encapsulate this however you'd like. – Keith Apr 03 '13 at 02:13
@krlmr. Just read your answer. I'm not really saying anything more. – Keith Apr 03 '13 at 02:15
1

Hint: `>> 0x10` is a bit of an overshoot. How much is `OtherIsUnderground / SelfIsUnderground`? -- Your solution is just another way to achieve the same result, with some advantages and some drawbacks. – krlmlr Apr 03 '13 at 02:17
Layers play a much larger role than what is shown in my question. For this reason, I cannot do what you say. – Mickael Bergeron Néron Apr 03 '13 at 10:02

score 0 · Answer 3 · answered Apr 03 '13 at 02:46

This is what you get for prematurely optimizing, using bitmasks and bitwise operations; there's no simple alternative.

Why not std::vector<Layer> selfLayers, otherLayers? (Or can you not have multiple layers - your code implies you can.)
Or maybe use a map from layer to requiredConditionBits?

Also, I don't understand why the Attack class is storing conditions for two Spirits. Who sets Attack::condition in the first place?

I would start by not using bit maps for things that are not bits or enums for things that are not enumerations. I would consider spending some time creating simple classes (such as Layer, Condition) and helper functions (Attack::getSourceLayer maybe? I'm too confused to be sure.) I would also review my class relationships to make sure that Attack really needs condition flags et cetera.

Layers play a much larger role than what is shown in my question. For this reason, I cannot do what you say. — Mickael Bergeron Néron, Apr 03 '13 at 10:01

score 0 · Answer 4 · answered Apr 03 '13 at 03:33

I would say abandon the Condition enumeration entirely, and use a struct instead. The conditions for the self and the target are seperate and should be treated as such

enum Layer : char //only takes 1 byte
{
    Ground      = 1<<0,
    Underground = 1<<1,
    Air         = 1<<2,
    Water       = 1<<3
};
struct Condition {
    Layer Self;
    Layer Target;
};
bool Attack::CanBeDone(Spirit* pSelf, Spirit* pTarget,Map* pMap) {
    return this->Condition.Self & pSelf->GetcurrentLayer() 
        && this->Condition.Target & pTarget->GetcurrentLayer();
}

Is there an alternative to this long list of "if argA == argB do ..."?

4 Answers4