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;
}
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
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.
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.
\$\endgroup\$
Commented
Mar 7, 2014 at 13:07
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.
\$\endgroup\$
Commented
Mar 7, 2014 at 14:24
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;
