# Movement code from a game

Could you suggest ways of writing this code better?

bool success = true;

if(x == 0)
{
if(do_up)
run_do_up();
else if(do_a)
run_do_a();
else if(do_n)
run_do_n();
else
success = false;
}
else if(x == 1)
{
if(do_n)
run_do_n();
else if(do_a)
run_do_a();
else
success = false;
}
else//x == 2
{
if(do_a)
run_do_a();
else if(do_n)
run_do_n();
else
success = false;
}

-
Welcome ! Please tell us more about what it is supposed to do. –  Josay Mar 7 at 9:18
Exactly what it looks. It's code from movement in a game. –  Luka Mar 7 at 9:18
And please tag this with the appropriate language tag. if this is pseudocode, then this question is off-topic. –  Jamal Mar 7 at 9:19
is it intentional that if both do_a and do_n are true, and do_up is not, you call run_do_n if x==1 and run_do_a otherwise? –  aragaer Mar 7 at 9:28
First suggestion is to use better names. Contrary to your prior comment, it does not look like movement code from a game - the only hint to it is do_up and run_do_up(), which do not have corresponding down, and there's no way to know what any of the other variables or functions are actually for (aside from success). –  Izkata Mar 7 at 17:27

switch clause is usually used to clean this up, at least at the outer level:

switch(x){
case 0:{
if(do_up)
run_do_up();
else if(do_a)
run_do_a();
else if(do_n)
run_do_n();
else
success = false;
break;
}
case 1:{
if(do_n)
run_do_n();
else if(do_a)
run_do_a();
else
success = false;
break;
}
default: {
if(do_a)
run_do_a();
else if(do_n)
run_do_n();
else
success = false;
}
}


Alternatively, you can just realize you have 3 things to run an put in the logic:

if(x==0 && do_up)run_do_up(); //the only case this happens
else //case 1 does n first, other cases do a first
if(do_a && (x!=1 || (x==1 && !do_n)))run_do_a();
else
if(do_n && (x==1 || (x!=1 && !do_a)))run_do_n();
else success=false;


Check if I messed it up. The main question is: do you want from the flow to be apparent which one is checked first (in which case just use switch statement) or you want to avoid duplication of calls. You can also go take the hybrid way and just set flags in the main if block and then just conditionally call them at the end. Like this

 ...
if(do_up) which_to_call=0;
else if(do_a) which_to_call=1;
else if(do_n) which_to_call=2;
else which_to_call=-1; //error flag

-
+1 for the 3 things with logic in the if statement. I had that idea also but didn't put enough effort in thinking about the logic. Nice answer! –  exilit Mar 7 at 12:54

I think I'd encode this into something on the order of a state machine--a table holding the conditions and actions associated with each, and a tiny bit of code to walk through those to carry out the tests:

#include <iostream>
#include <functional>

#ifdef TEST
bool do_n, do_a, do_up;
bool success;
bool T = true;

void run_do_up() { std::cout << "did up\n"; }
void run_do_a() { std::cout << "did a\n"; }
void run_do_n() { std::cout << "did n\n"; }
#endif

struct conditional_action {
bool const &condition;
std::function <void()> action;

template <class F>
conditional_action(bool const &b, F &a) : condition(b), action(a) {}

conditional_action() : condition(T), action([&]{success = false; }) {}
};

conditional_action actions[4][3] {
{ { do_up, run_do_up }, { do_a, run_do_a }, { do_n, run_do_n } },
{ { do_n, run_do_n }, { do_a, run_do_a } },
{ { do_a, run_do_a }, { do_n, run_do_n } }
};

void exec(size_t x) {
for (auto const &a : actions[x])
if (a.condition) {
a.action();
return;
}
}

#ifdef TEST

int main() {
do_n = true;
exec(0);

do_a = true;
exec(2);
}
#endif


Depending on viewpoint, you might want to add a { T, [&]{success = false; }} to the end of each row in the actions table. That's what the default ctor will fill those entries with anyway, but there's a fair argument to be made that since it was explicit in the original logic, it should remain explicit here as well.

There is probably at least a little argument to be made that this is kind of a borderline case. A case where there were more conditions and actions would favor this technique more strongly. Whether really it makes sense when you have only three conditions and actions may be open to a little more question.

-
Isn't that to much for such a small task? –  exilit Mar 7 at 12:50
@exilit: Well, let's compare it to your answer. The combined total of exec, actions and conditional_action is exactly the same number of lines of code as you give in your answer. Your answer got that short, however, by breaking the original logic (do_up should not be considered when x==2). So, mine is not only much more scalable, but even for this small case is already the shortest code posted that actually works. It mostly looks longer because I added in demo/mock/test code. –  Jerry Coffin Mar 7 at 13:07
I am not talking about loc. What I am talking about is the time and effort needed for the logic that sits behind. –  exilit Mar 7 at 13:43
@exilit: What time and effort? The effort of a single for loop or of a struct of two whole members? The effort to fully understand and verify something like if(do_a && (x!=1 || (x==1 && !do_n)))run_do_a(); alone exceeds the effort involved in writing and verifying my entire program. –  Jerry Coffin Mar 7 at 14:24
@exilit: Maybe I just think in terms of larger, more complex problems, but I have to admit that I just don't see the difficulty or complexity. –  Jerry Coffin Mar 7 at 15:07

If you write a few boolean returning wrapper functions:

bool cond_do_a(bool do) {
if (do) run_do_a();
return do;
}
bool cond_do_n(bool do) {
if (do) run_do_n();
return do;
}
bool cond_do_up(bool do) {
if (do) run_do_up();
return do;
}


Than you can express the code with a few simple logical or operators:

switch(x){
case 0: success = cond_do_up(do_up) || cond_do_a(do_a) || cond_do_n(do_n);
break;
case 1: success = cond_do_n(do_n) || cond_do_a(do_a);
break;
case 2: success = cond_do_a(do_a) || cond_do_n(do_n);
break;
default:
success = false;
}

-

You could try anything like this:

bool success = true;

if(x == 1)
{
if(do_n)
run_do_n();
else if(do_a)
run_do_a();
else
success = false;
}
else//x == 2 || x == 0
{
if(do_up && x == 0)
run_do_up();
else if(do_a)
run_do_a();
else if(do_n)
run_do_n();
else
success = false;
}

-

Put the maximum number of cases together in the same conditional. In this case, it means lumping values of x and do_...

bool success = true;
if (x == 0 && do_up) run_do_up();
else if (x == 1 && do_n) run_do_n();
else if (do_a) run_do_a();
else if (do_n) run_do_n();
else success = false;

-