CODEWITHSUNDEEP
c++if-statement
SinisterMJ
SinisterMJ

Reputation: 3509

Conditional assignment in if clause

I have a code which I do not want to duplicate too much, it looks something like this:

myEnum setting;

if (((input == "Enum0") && ((setting = myEnum_0) == setting)) ||
    ((input == "Enum1") && ((setting = myEnum_1) == setting)) ||
    ((input == "Enum2") && ((setting = myEnum_2) == setting)))
{
    doActionWith(setting);
}

This way I don't have to check if the input was something totally different (if I just assigned with

if (input=="Enum0")
    setting = myEnum_0;

the code wouldn't know if the enum was actually set, or not.

Now I wonder, is there a more elegant solution for the thing at the end?

The && ((setting = myEnum_x) == setting) ?

Upvotes: 2

Views: 70

Answers (3)

Aziuth
Aziuth

Reputation: 3902

I'd go with this:

//in the part of the code where you declare myEnum,
//if in a header, you might want to declare this inline
myEnum toMyEnum(const string input){
    if(input == "Enum0")
        return myEnum_0;
    ...
    return undefinedMyEnum;
}

and then you go with

myEnum setting = toMyEnum(input);
if(setting != undefinedMyEnum){
    doActionWith(setting);
}

This way, you have clear responsibilities. You simply take a string and transform it into your enumeration. Your code with the side effect is difficult to read, avoid such "tricks".

edit: on a second thought, right now my code produces a silent error, that is most likely not that good. You might want to replace return undefinedMyEnum; with something such as an EXIT or throw error or an apropriate assert.

Upvotes: 2

Jonas
Jonas

Reputation: 7017

Personalty, I would go with a design like the one suggested by Aziuth. But if you want to keep you current design, the you could do this:

bool doStuff = true;

if (input=="Enum0")
    setting = myEnum_0;
else if (input=="Enum1")
    setting = myEnum_1;
else if (input=="Enum2")
    setting = myEnum_2;
else
{
    doStuff = false;
}

if (doStuff)
{
    doActionWith(setting);
}

Upvotes: 0

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726489

Apart from the fact that (setting = myEnum_0) == setting uses setting twice without a sequence point, your code is very hard to understand. Even if you fix undefined behavior, like this, (setting = myEnum_0) == myEnum_0, your code would remain a puzzle to anyone maintaining it.

A better approach is to make a helper function, and use it three times in the conditional:

if (tryParse(input, "Enum0", setting, Enum0)
||  tryParse(input, "Enum1", setting, Enum1)
||  tryParse(input, "Enum2", setting, Enum2)) {
}

with tryParse defined to return true on success and false otherwise.

Upvotes: 2

Related Questions