How do you handle huge if-conditions?

Question

It's something that's bugged me in every language I've used, I have an if statement but the conditional part has so many checks that I have to split it over multiple lines, use a nested if statement or just accept that it's ugly and move on with my life.

Are there any other methods that you've found that might be of use to me and anybody else that's hit the same problem?

Example, all on one line:

if (var1 = true && var2 = true && var2 = true && var3 = true && var4 = true && var5 = true && var6 = true)
{

Example, multi-line:

if (var1 = true && var2 = true && var2 = true
 && var3 = true && var4 = true && var5 = true
 && var6 = true)
{

Example-nested:

if (var1 = true && var2 = true && var2 = true && var3 = true)
{
     if (var4 = true && var5 = true && var6 = true)
     {

Answer 1

First, I'd remove all the '= true' parts, that would make it 50% shorter ;)

When I have big condition I search for the reasons. Sometimes I see I should use polymorphism, sometimes I need to add some state object. Basically, it implies a refactoring is needed (a code smell).

Sometimes I use De-Morgan's laws to simplify boolean expressions a bit.

Answer 2

I resort to separate boolean values:

Bool cond1 == (var1 && var2);
Bool cond2 == (var3 && var4);

if ( cond1 && cond2 ) {}

Answer 3

I've seen a lot of people and editors either indenting each condition in your if statement with one tab, or matching it up with the open paren:

if (var1 == true
    && var2 == true
    && var3 == true
   ) {
    /* do something.. */
}

I usually put the close paren on the same line as the last condition:

if (var1 == true
    && var2 == true
    && var3 == true) {
    /* do something.. */
}

But I don't think this is quite as clean.

Answer 4

I'll often split these up into component boolean variables:

bool orderValid = orderDate < DateTime.Now && orderStatus != Status.Canceled;
bool custValid = customerBalance == 0 && customerName != "Mike";
if (orderValid && custValid)
{
...

Answer 5

Separate the condition in several booleans and then use a master boolean as the condition.

bool isOpaque = object.Alpha == 1.0f;
bool isDrawable = object.CanDraw && object.Layer == currentLayer;
bool isHidden = hideList.Find(object);

bool isVisible = isOpaque && isDrawable && ! isHidden;

if(isVisible)
{
    // ...
}

Better yet:

public bool IsVisible {
    get
    {
        bool isOpaque = object.Alpha == 1.0f;
        bool isDrawable = object.CanDraw && object.Layer == currentLayer;
        bool isHidden = hideList.Find(object);

        return isOpaque && isDrawable && ! isHidden;
    }
}

void Draw()
{
     if(IsVisible)
     {
         // ...
     }
}

Make sure you give your variables name that actualy indicate intention rather than function. This will greatly help the developer maintaining your code... it could be YOU!

Answer 6

Well, first off, why not:

if (var1 && var2 && var2 && var3 && var4 && var5 && var6) {
...

Also, it's very hard to refactor abstract code examples. If you showed a specific example it would be easier to identify a better pattern to fit the problem.

It's no better, but what I've done in the past: (The following method prevents short-circuiting boolean testing, all tests are run even if the first is false. Not a recommended pattern unless you know you need to always execute all the code before returning -- Thanks to ptomato for spotting my mistake!)

boolean ok = cond1;
ok &= cond2;
ok &= cond3;
ok &= cond4;
ok &= cond5;
ok &= cond6;

Which is the same as: (not the same, see above note!)

ok = (cond1 && cond2 && cond3 && cond4 && cond5 && cond6);

Answer 7

I like to break them down by level, so I'd format you example like this:

if (var1 = true
 && var2 = true
 && var2 = true
 && var3 = true
 && var4 = true
 && var5 = true
 && var6 = true){

It's handy when you have more nesting, like this (obviously the real conditions would be more interesting than "= true" for everything):

if ((var1 = true && var2 = true)
 && ((var2 = true && var3 = true)
  && (var4 = true && var5 = true))
 && (var6 = true)){

Answer 8

Check out Implementation Patterns by Kent Beck. There is a particular pattern I am thinking of that may help in this situation... it is called "Guards". Rather than having tons of conditions, you can break them out into a guard, which makes it clear which are the adverse conditions in a method.

So for example, if you have a method that does something, but there are certain conditions where it shouldn't do something, rather than:

public void doSomething() {
    if (condition1 && condition2 && condition3 && condition4) {
        // do something
    }
}

You could change it to:

public void doSomething() {
    if (!condition1) {
        return;
    }

    if (!condition2) {
        return;
    }

    if (!condition3) {
        return;
    }

    if (!condition4) {
        return;
    }

    // do something
}

It's a bit more verbose, but a lot more readable, especially when you start having weird nesting, the guard can help (combined with extracting methods).

I HIGHLY recommend that book by the way.

Answer 9

I'm surprised no one got this one yet. There's a refactoring specifically for this type of problem:

http://www.refactoring.com/catalog/decomposeConditional.html

Answer 10

There are two issues to address here: readability and understandability

The "readability" solution is a style issue and as such is open to interpretation. My preference is this:

if (var1 == true && // Explanation of the check
    var2 == true && // Explanation of the check
    var3 == true && // Explanation of the check
    var4 == true && // Explanation of the check
    var5 == true && // Explanation of the check
    var6 == true)   // Explanation of the check
    { }

or this:

if (var1 && // Explanation of the check
    var2 && // Explanation of the check
    var3 && // Explanation of the check
    var4 && // Explanation of the check
    var5 && // Explanation of the check
    var6)   // Explanation of the check
    { }

That said, this kind of complex check can be quite difficult to mentally parse while scanning the code (especially if you are not the original author). Consider creating a helper method to abstract some of the complexity away:

/// <Summary>
/// Tests whether all the conditions are appropriately met
/// </Summary>
private bool AreAllConditionsMet (
    bool var1,
    bool var2,
    bool var3,
    bool var4,
    bool var5,
    bool var6)
{
    return (
        var1 && // Explanation of the check
        var2 && // Explanation of the check
        var3 && // Explanation of the check
        var4 && // Explanation of the check
        var5 && // Explanation of the check
        var6);  // Explanation of the check
}

private void SomeMethod()
{
    // Do some stuff (including declare the required variables)
    if (AreAllConditionsMet (var1, var2, var3, var4, var5, var6))
    {
        // Do something
    }
}

Now when visually scanning the "SomeMethod" method, the actual complexity of the test logic is hidden but the semantic meaning is preserved for humans to understand at a high-level. If the developer really needs to understand the details, the AreAllConditionsMet method can be examined.

This is formally known as the "Decompose Conditional" refactoring pattern I think. Tools like Resharper or Refactor Pro! can making doing this kind of refactoring easy!

In all cases, the key to having readable and understandable code is to use realistic variable names. While I understand this is a contrived example, "var1", "var2", etc are not acceptable variable names. They should have a name which reflects the underlying nature of the data they represent.

Answer 11

If you happen to be programming in Python, it's a cinch with the built-in all() function applied over the list of your variables (I'll just use Boolean literals here):

>>> L = [True, True, True, False, True]
>>> all(L) # True, only if all elements of L are True.
False
>>> any(L) # True, if any elements of L are True.
True

Is there any corresponding function in your language (C#? Java?). If so, that's likely the cleanest approach.

Answer 12

If you do this:

if (var1 == true) {
    if (var2 == true) {
        if (var3 == true) {
            ...
        }
    }
}

Then you can also respond to cases where something isn't true. For example, if you're validating input, you could give the user a tip for how to properly format it, or whatever.

Answer 13

McDowell,

You are correct that when using the single '&' operator that both sides of the expression evaluate. However, when using the '&&' operator (at least in C#) then the first expression to return false is the last expression evaluated. This makes putting the evaulation before the FOR statement just as good as any other way of doing it.

Answer 14

As others have mentioned, I would analyze your conditionals to see if there's a way you can outsource it to other methods to increase readability.

Answer 15

@tweakt

It's no better, but what I've done in the past:

boolean ok = cond1; ok &= cond2; ok &= cond3; ok &= cond4; ok &= cond5; ok &= cond6;

Which is the same as:

ok = (cond1 && cond2 && cond3 && cond4 && cond5 && cond6);

Actually, these two things are not the same in most languages. The second expression will typically stop being evaluated as soon as one of the conditions is false, which can be a big performance improvement if evaluating the conditions is expensive.

For readability, I personally prefer Mike Stone's proposal above. It's easy to verbosely comment and preserves all of the computational advantages of being able to early out. You can also do the same technique inline in a function if it'd confuse the organization of your code to move the conditional evaluation far away from your other function. It's a bit cheesy, but you can always do something like:

do {
    if (!cond1)
       break;
    if (!cond2)
       break;
    if (!cond3)
       break;
    ...
    DoSomething();
} while (false);

the while (false) is kind of cheesy. I wish languages had a scoping operator called "once" or something that you could break out of easily.

Answer 16

Try looking at Functors and Predicates. The Apache Commons project has a great set of objects to allow you to encapsulate conditional logic into objects. Example of their use is available on O'reilly here. Excerpt of code example:

import org.apache.commons.collections.ClosureUtils;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.collections.functors.NOPClosure;

Map predicateMap = new HashMap();

predicateMap.put( isHonorRoll, addToHonorRoll );
predicateMap.put( isProblem, flagForAttention );
predicateMap.put( null, ClosureUtils.nopClosure() );

Closure processStudents = 
    ClosureUtils.switchClosure( predicateMap );

CollectionUtils.forAllDo( allStudents, processStudents );

Now the details of all those isHonorRoll predicates and the closures used to evaluate them:

import org.apache.commons.collections.Closure;
import org.apache.commons.collections.Predicate;

// Anonymous Predicate that decides if a student 
// has made the honor roll.
Predicate isHonorRoll = new Predicate() {
  public boolean evaluate(Object object) {
    Student s = (Student) object;

    return( ( s.getGrade().equals( "A" ) ) ||
            ( s.getGrade().equals( "B" ) && 
              s.getAttendance() == PERFECT ) );
  }
};

// Anonymous Predicate that decides if a student
// has a problem.
Predicate isProblem = new Predicate() {
  public boolean evaluate(Object object) {
    Student s = (Student) object;

    return ( ( s.getGrade().equals( "D" ) || 
               s.getGrade().equals( "F" ) ) ||
             s.getStatus() == SUSPENDED );
  }
};

// Anonymous Closure that adds a student to the 
// honor roll
Closure addToHonorRoll = new Closure() {
  public void execute(Object object) {
    Student s = (Student) object;

    // Add an award to student record
    s.addAward( "honor roll", 2005 );
    Database.saveStudent( s );
  }
};

// Anonymous Closure flags a student for attention
Closure flagForAttention = new Closure() {
  public void execute(Object object) {
    Student s = (Student) object;

    // Flag student for special attention
    s.addNote( "talk to student", 2005 );
    s.addNote( "meeting with parents", 2005 );
    Database.saveStudent( s );
  }
};

Answer 17

I like to break each condition into descriptive variables.

bool isVar1Valid, isVar2Valid, isVar3Valid, isVar4Valid;
isVar1Valid = ( var1 == 1 )
isVar2Valid = ( var2.Count >= 2 )
isVar3Valid = ( var3 != null )
isVar4Valid = ( var4 != null && var4.IsEmpty() == false )
if ( isVar1Valid && isVar2Valid && isVar3Valid && isVar4Valid ) {
     //do code
}

Answer 18

Steve Mcconell's advice, from Code Complete: Use a multi-dimensional table. Each variable serves as an index to the table, and the if statement turns into a table lookup. For example if (size == 3 && weight > 70) translates into the table entry decision[size][weight_group]

Answer 19

If I was doing it in Perl, This is how I might run the checks.

{
  last unless $var1;
  last unless $var2;
  last unless $var3;
  last unless $var4;
  last unless $var5;
  last unless $var6;

  ... # Place Code Here
}

If you plan on using this over a subroutine replace every instance of last with return;

Answer 20

In reflective languages like PHP, you can use variable-variables:

$vars = array('var1', 'var2', ... etc.);
foreach ($vars as $v)
    if ($$v == true) {
        // do something
        break;
    }