Conditional assignment in if clause

Question

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) ?

Isn't `((setting = myEnum_0) == setting))` undefined behaviour? — LogicStuff, Jun 07 '17 at 10:56
How would I do it otherwise, have the right side only checked if the left evaluates true, but work with Enum = 0 just as well as Enum = 5? — SinisterMJ, Jun 07 '17 at 11:01

Aziuth · Answer 1 · 2017-06-07T11:12:00.857

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.

score 2 · Accepted Answer · answered Jun 07 '17 at 11:03

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.

score 0 · Answer 3 · answered Jun 07 '17 at 11:05

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);
}

Conditional assignment in if clause

3 Answers3