Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
4  
Welcome ! Please tell us more about what it is supposed to do. –  Josay Mar 7 at 9:18
1  
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
1  
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
2  
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
show 4 more comments

5 Answers 5

up vote 5 down vote accepted

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
share|improve this answer
1  
+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
add comment

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.

share|improve this answer
    
Isn't that to much for such a small task? –  exilit Mar 7 at 12:50
2  
@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
1  
@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
1  
@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
show 3 more comments

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;
}
share|improve this answer
add comment

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;
}
share|improve this answer
add comment

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;
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.