Movement code from a game

Question

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

Answer 1

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

• exilit: +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!

Answer 2

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.

• exilit: Isn't that to much for such a small task?
• Jerry Coffin: @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.
• exilit: I am not talking about loc. What I am talking about is the time and effort needed for the logic that sits behind.
• Jerry Coffin: @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: @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.

Answer 3

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

Answer 4

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

Answer 5

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;