How would you shorten this so that action1 and action2 only show up once in code?

Question

if (x() > 10)
{
    if (y > 5)
        action1(p1, p2, p3, p4);
    else
        action2(p1, p2);
}
else
{
    if (z > 2)
        action1(p1, p2, p3, p4);
    else
        action2(p1, p2);
}

I real project on mine, action1 and action2 are actually 2-3 lines of code and those functions that are invoked take 6-8 parameters in total, so writing them as a single function doesn't seem right.

UPDATE: I forgot to mention this, and now I see many answers just don't work. x() is expensive operation and has side-effects, so it should not be called twice.

Answer 1

You could always do:

if ((x > 10) ? (y >5) : (z > 2)) action1
else action2

For completeness sake, any if p then q else r statement can be expressed logically as (!p && r) || q. So, we can express the original statement as:

a = x > 10
b = y > 5
c = z > 2
(!a && (!c && action2) || ((!b && action2) || action1))

Which you can expand out as:

(!a && !c && action2) || 
(!a &&  c && action1) || 
( a && !b && action2) || 
( a &&  b && action1)

If you collect action1 to one side, you get:

( a &&  b && action1) ||
(!a &&  c && action1) || 

( a && !b && action2) || 
(!a && !c && action2)

It really expands to:

( a &&  b &&  c && action1) ||
( a &&  b && !c && action1) ||
(!a &&  b &&  c && action1) || 
(!a && !b &&  c && action1) || 

( a && !b &&  c && action2) || 
( a && !b && !c && action2) || 
(!a &&  b && !c && action2) ||
(!a && !b && !c && action2)

And from that we can see we can simplify it to:

( a &&  b &&  c && action1) ||
( a &&  b && !c && action1) ||
(!a &&  b &&  c && action1) || 
(!a && !b &&  c && action1) || 
action2

Since all paths leading to action2 are the negation of any path leading to action1, and we can further reduce it to:

( a &&  b && action1) ||
(!a &&  c && action1) || 
action2

Which can itself be reduced to:

((( a &&  b &&) || (!a &&  c)) && action1) || 
action2

Which can then be written as:

if ((a && b) || (!a && c)) action1
else action2

Which becomes: if ((x > 10 && y > 5) || (!(x > 10) && z > 2)) action1 else action2

Which is what we get anyways.

Answer 2

I like the original, verbose version. But only if it follows the inside logic of the system that the code is describing. Otherwise, maybe it should be the "opposite":

int xRes = x();
if (y > 5) 
{
    if (xRes > 10)
        action1(p1, p2, p3, p4);
    else
        action2(p1, p2);
}
else if (z > 2)
{
    if (xRes > 10)
        action1(p1, p2, p3, p4);
    else
        action2(p1, p2);
}
else
{
    action2(p1, p2);
}

Anyway, if you are interested in the shortest solution, this can be it:

((x>10 && y>5) || (x<=10 && z>2)) ? action1(p1, p2, p3, p4) : action2(p1, p2);

Answer 3

if ((x > 10 && y > 5) || (x <= 10 && z > 2))
   action1(p1, p2, p3, p4);
else
   action2(p1, p2);

Answer 4

if (should_do_action1(x(), y, z))
    action1(p1, p2, p3, p4);
else
    action2(p1, p2);

Answer 5

If you want to do it with just 1 if statements, I think it would be:

if ((x > 10) && (y > 5)) || ((x <= 10) && (z > 2))
    action1(p1, p2, p3, p4);
else
    action2(p1, p2);

Answer 6

//Yeah, I know this is wrong. Explanation below.
if ((x() > 10 && y > 5) || (x() <= 10 && z > 2))
    action1(p1, p2, p3, p4);
else
    action2(p1, p2);

[edit] Changed code because my conditional logic was wrong.

But personally, for readability's sake I prefer your more verbose version.

[edit2] As the comments have noticed, I seem to be having trouble getting this exactly right. I would note that this is because the way I would go about it is pretty much how the question originally states. I prefer verbosity over strings of boolean operators, because in my opinion it makes the logic of the operation much easier to visualize.

Answer 7

I can't think of a way to express it more clearly or simply. It could be made shorter, but it would make it harder to understand.

Answer 8

You know, after seeing some of the responses here, I'd have to say your original version is more readable, and certainly more understandable. The only change I'd make would be to add a little comment on the else line stating that the else case is for x <= 10.

I don't think shortening code is really that much of an issue anymore for pretty much any reason. I know if you're an old-school coder, you like to have everything as concise as possible, but we all have 100+Gb drives and compilers all collapse any of these alternatives to the same code anyway, so why not opt for readability? My 2 cents :)

Answer 9

bool condition_satisfied = (x() > 10 ? y > 5 : z > 2);
if (condition_satisfied)
    action1(p1, p2, p3, p4);
else
    action2(p1, p2);

Or, alternatively, what Roger Lipscombe answered.

Answer 10

since x() is expensive, cache the result. Then combine the preconditions on action1:

int x = x();
if ((x>10 && y>5) || (x<=10 && z>2))
{
    action1(p1,p2,p3,p4);
}
else 
{
    action2(p1,p2);
}

Answer 11

var valX= x();
if ((valX > 10  && y>5) || (valX<=10 && z>2))
{

        action1(p1, p2, p3, p4);
}
else
{  
        action2(p1, p2);  
}