What's the best way to get rid of nested ifs in code while checking for conditions?

Question

I'm developing a BlackBerry app in Java and I have an Options class where all user settings are stored. The problem is I need to check some conditions in order to know how to react. As I keep adding more features, more GUI options are shown to the user, more settings are stored in the Options class and more conditions need to be checked for.

Take the following code for example:

private void doCallMonitoring(int callId){
    /*This is the part that I want to avoid. Having
      multiple nested ifs. Here's just two conditions
      but as I add more, it will get unmantainable
      very quickly.*/
    if(Options.isActive().booleanValue()){
        callTime = new Timer();
        TimerTask callTimeTask = new TimerTask(){
            public void run(){
                callTimeSeconds++;
     if((callTimeSeconds == Options.getSoftLimit().intValue()) && (Phone.getActiveCall().getStatus() == PhoneCall.STATUS_CONNECTED)){
                    injectDTMFTone(Phone.getActiveCall());
     }else if((callTimeSeconds >= Options.getHardLimit().intValue()) && (Phone.getActiveCall().getStatus() == PhoneCall.STATUS_CONNECTED)){
                    injectEndCall();
                }
             }
        };     
        callTime.schedule(callTimeTask, 0,1000);
    }else{
    System.out.println("Service not active");
    }
}

How I would want it to work is to verify all options with a single call and from there determine the curse of action. How can I achieve such a design?

Answer 1

You can use "extract method" refactoring and have all those checks into a one "readable" condition.

See this related answer Is bit lengthy but the point is to replace constructs like this:

       }else if((callTimeSeconds >= Options.getHardLimit().intValue()) && (Phone.getActiveCall().getStatus() == PhoneCall.STATUS_CONNECTED)){
                injectEndCall();
            }
         }

For something like this:

       ....
       }else if(shouldInjectEndCall() ){
                injectEndCall();
            }
         }
       ...

Remember objects do have state and may use other object to help them do its work.

Answer 2

Other option is to do some sort of "replace conditional with polymorphism".

Although it seems like just writing more code you could replace all those rules with a "validator" object and have all the validations in some array and loop through them.

Something like this scratch code.

  private void doCallMonitoring(int callId){
     // Iterate the valiators and take action if needed. 

      for( Validation validation : validationRules ) { 
          if( validation.succeed() ) { 
              validation.takeAction();
          }
      }
   }

And you implement them like this:

abstract class Validation { 

      public boolean suceed();
      public void takeAction();
}

class InjectDTMFToneValidation extends Validation { 
    public boolean suceed() { 
        return (callTimeSeconds == Options.getSoftLimit().intValue()) 
               && (Phone.getActiveCall().getStatus() == PhoneCall.STATUS_CONNECTED)
     }
     public void takeAction() { 
         injectDTMFTone(Phone.getActiveCall());
     }
}

class InjectEndCallValidation extends Validation { 
    public boolean suceed() { 
        return (callTimeSeconds >= Options.getHardLimit().intValue()) 
                && (Phone.getActiveCall().getStatus() == PhoneCall.STATUS_CONNECTED)
     }
     public void takeAction() { 
         injectEndCall();
     }
}

And finally install them in a list:

private List<Validation> validationRules = new ArrayList<Validation>();{
   validationrules.add( new InjectDTMFToneValidation() );
   validationrules.add( new InjectEndCallValidation () );
   ...
   ...
}

The idea here is to move the logic to subclasses. Of course, you will get a better structure and probably suceed and takeAction could be replaced for other more meaningful methods, the objective is to pull the validation from where it is.

It becomes more abstract? .. yes.

BTW, I why do yo use Options and Phone classes invoking their static methods instead of using instances?

Answer 3

Another option is to make methods such as injectDMTFTone() check to see if they want to handle that condition, and return true or false depending on if it was handled or not.

For instance:

public void run() {
    callTimeSeconds++;
    do {
        if (handleInjectDMTFTone())
            break;
        if (handleInjectEndCall())
            break;
    } while(false);

    callTime.schedule(callTimeTask, 0,1000);
}

boolean handleInjectDMTFTone() {
    if ((callTimeSeconds != Options.getSoftLimit().intValue()) ||
        (Phone.getActiveCall().getStatus() != PhoneCall.STATUS_CONNECTED))
        return false;

    injectDTMFTone(Phone.getActiveCall());
    return true;
}

boolean handleInjectEndCall() {

    if ((callTimeSeconds < Options.getHardLimit().intValue()) ||
        (Phone.getActiveCall().getStatus() != PhoneCall.STATUS_CONNECTED))
        return false;

    injectEndCall();
    return true;
}

Of course, instead of calling another injectDMTFTone() method or injectEndCall() method, you would just inline that logic right in those methods. In that way you've grouped all the logic of how and when to deal with those conditions in the same place.

This is one of my favorite patterns; use if statements as close to the top of methods as makes sense to eliminate conditions and return. The rest of the method is not indented many levels, and is easy and straightforward to read.

You can further extend this by creating objects that all implement the same interface and are in a repository of handlers that your run method can iterate over to see which will handle it. That may or may not be overkill to your case.

Answer 4

All those answers are probably better OO answers. For once I'll go for the quick and dirty answer.

I really like to simplify complex nested ifs like that by inverting them.

if(!Options.isActive().booleanValue()) {
    System.out.println("Service not active");
    return;
}
the rest...

I know some people don't like mid-method returns, but when you are validating entry conditions that has always been an awesome pattern for me, I've never regretted using it.

It really simplifies the way your method looks.

If you write methods longer than a screen, don't do this or write a large comment pointing it out--It's too easy to lose the return statement and forget you did it. Better yet, don't write methods longer than a screen.