How would I simplify this code?

Question

boolean f(boolean A, boolean B, boolean C, boolean D, boolean E)
{
  if (A)
  {
    k();
    if (B)
    {
      m();
      if (C)
      {
        n();
        if (D)
        {
          p();
          if (E)
          {
            q();
            return true;
          }
          else
          {
            r();
            return false;
          }
        }
        else
        {
          s();
          return false;
        }
      }
      else
      {
        t();
        return false;
      }
    }
    else
    {
      v();
      return false;
    }
  }
  else
  {
    w();
    return false;
  }
}

"Simplify" in what way? This probably belongs on http://codereview.stackexchange.com — Lightness Races in Orbit, Nov 24 '11 at 20:19

MK. · Answer 1 · 2011-11-24T20:19:02.717

Without knowing more about the problem you are solving, I would rewrite it as

boolean f(boolean A, boolean B, boolean C, boolean D, boolean E)
{
  if (A) k();
  if (A && B) m();
  if (A && B && C) n();
  if (A && B && C && D) p();
  if (A && B && C && D && E) { q(); return true; }
  if (A && B && C && D && !E) { r(); return false; }
  if (A && B && C && !D) { s(); return false; }
  if (A && B && !C) { t(); return false; }
  if (A && !B) { v(); return false; }
  if (!A) { w(); return false; }
}

This, in my opinion, makes it somewhat easier to follow the scenarios.
However this is still absolutely horrible. What you most likely want is some kind of algorithm pattern where the different behaviors are impemented as different classes which implement the same interface and you would select the behavior based on polymorphism or the algorithm would be injected during object creation.
Basically every method taking more than one boolean argument is a code smell.

score 3 · Accepted Answer · answered Nov 24 '11 at 20:12

Probably only by flattening the ifs by evaluating the conditions more than once:

if (A) k(); else w();
if (A && B) m(); else if(A && !B) v();
if (A && B && C) n(); else if (A && B && !C) t();
if (A && B && C && D) p(); else if (A && B && C && !D) s();
if (A && B && C && D && E) q(); else if (A && B && C && D && !E) r();

return (A && B && C && D && E);

score 0 · Answer 3 · answered Dec 06 '12 at 23:59

I was asked to optimize this code during a recent job interview.

Here's the version of code I came up with:

boolean f(boolean A, boolean B, boolean C, boolean D, boolean E )
{
    boolean success = false;

    // I use "==false" because it's more readable than "if !A"
    if(A == false)
    {
        w();
    } else {
        if(B == false)
        {
            v();
        } else {
            m();
            if(C == false)
            {
                n();
                if(D == false)
                {
                    s();
                } else {
                    if(E == false)
                    {
                        r();
                    } else {
                        q();
                        success = true;
                    }
                }
            }
        }
    }

    // this will be "false" in all cases except one
    return (success);
}

And my logic behind my answer was to try to maintain readability while reducing the number of "returns".

Here is the answer the hiring guy was really looking for:

boolean f(Boolean A, Boolean B, Boolean C, Boolean D, Boolean E)
{
    boolean result = false;

    do
    {
        if (!A)
        {
            w();
            break;
        }

        k();
        if (!B)
        {
            v();
            break;
        }

        m();
        if (!C)
        {
            t();
            break;
        }

        n();
        if (!D)
        {
            s();
            break;
        }

        p();
        if (!E)
        {
            r();
            break;
        }

        // All conditions satisfied
        result = true;

    } while (false);

    return result;
}

This uses the wily do-once-and-only-once loop, with the idea of "break"-ing out when some condition fails.

andyhasit · Answer 4 · 2011-11-24T22:15:12.960

0

If the order in which the methods are called isn't important, then:

failedOnce = false
for ar as Array in [
    (A, K, W)
    (B, M, V)
    (C, N, T)
    (D, P, S)
    (E, Q, R)
    ]:
    if ar[0]:
        ar[1].Invoke()
    else:
        ar[2].Invoke()
        break
        failedOnce = false

return not failedOnce

edited Nov 24 '11 at 22:15

answered Nov 24 '11 at 20:20

andyhasit

14,137
7
49
51

1

this optimizes for length, yes, but doesn't make it particularly easy to read. – MK. Nov 24 '11 at 20:22
That would call a `B`'s method even if `A` is `false`, wouldn't it? – GSerg Nov 24 '11 at 20:27
@MK, I would say this far easier to read, and easier to maintain as the logic is in one place, and the linking of conditions to functions very simply laid out in a list of arrays, and its the latter you'll most likely want to change. – andyhasit Nov 24 '11 at 22:20
@GSerg: yes you were right, edited so it should be correct now. – andyhasit Nov 24 '11 at 22:20

How would I simplify this code?

4 Answers4