3

The use case is there is a set of methods which need to be executed based on whether the previous one has returned true or not. For example:

class Test {
    boolean method1() {...}
    boolean method2() {...}
    boolean method3() {...}

    ...

    void callAll() {
        if(method1()) {
             if(method2() {
                 if(method3() {
                     ...
                 }
             }
        } else {
            error();
        }
    }
}

There has to be an else for all the ifs. Is there a better way of handling this scenario?

Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
cmn
  • 383
  • 3
  • 12
  • 1
    Do these methods have side effects or are they functions? –  Aug 06 '18 at 08:45
  • 4
    `if (method1() && method2() && method3())`? – Mick Mnemonic Aug 06 '18 at 08:47
  • 1
    @MickMnemonic *If* these are functions without side effects, that's what I would recommend. –  Aug 06 '18 at 08:48
  • 1
    "There has to be an else for all the ifs" - do you mean an else for *each* if ? – Brian Agnew Aug 06 '18 at 08:49
  • 1
    @intentionallyleftblank, how does having side effects matter here if the requirement was simply about only executing if the previous method returned a truthy value, i.e. short-circuiting. – Mick Mnemonic Aug 06 '18 at 08:50
  • In the above code not having side effects indeed does not matter. But if there was al `else` based on `!method1()`, your short circuit `if` would have to be repeated as an `if else` with different `!`s. This would then execute `method1` again. –  Aug 06 '18 at 09:11
  • @BrianAgnew yes, each if must have an else. This is to know which method in the chain has failed. – cmn Aug 06 '18 at 10:44

7 Answers7

8

I would just do it like this:

void callAll(){
    if(method1() && method2() && method3()){
        // all passed
    } else {
        error();    
    }
}

Java short-circuits the && logical operation so failure in a previous method here will prevent running the next one.

If in error() you need to know which of the methods failed, you could declare an error message field for storing the information within the class and set its value corresponding the failure:

private String errorMessage;

//...

boolean method2() {

    // something went wrong
    errorMessage = "Failed to do method2 stuff";
}

Are more elegant way to achieve the same would be to use the Chain of responsibility design pattern and encapsulate the boolean methods in their own handler objects. Doing this would however require more refactoring to the code you currently have and more information about your specific use case.

Mick Mnemonic
  • 7,808
  • 2
  • 26
  • 30
  • I tried this but how can I know which method has failed in the chain? – cmn Aug 06 '18 at 10:43
  • @cmn, I have edited the answer. To get better answers, it would be important to include all relevant information in the question, such as what `error()` does, is a failure in one of the methods exceptional (or "normal") and what kind of informaiton from the failure is relevant. – Mick Mnemonic Aug 06 '18 at 12:17
4

It's easy enough to write your own varargs method to do this:

public static void run(Supplier<Boolean>... methods) {
    for (Supplier<Boolean> method : methods) {
        if (!method.get()) return;
    }
}

Sample usage:

run(this::method1, this::method2, this::method3);
Michael
  • 41,989
  • 11
  • 82
  • 128
  • If these methods don't have side-effects, you should amend the above to return a boolean indicating all methods have run (or not, or provide an additional function to execute if all have succeeded), Otherwise it doesn't achieve anything? – Brian Agnew Aug 06 '18 at 08:51
  • @Michael I couldnt use the @ user id there because you didnt comment yet. – GhostCat Aug 06 '18 at 11:27
1

You can use some form of Observable pattern for these kind of thins too. In most normal cases it seems a bit silly to implement it but otherwise a great way to decouple code from control structures if you have a lot of these. Note that ObservableBoolean is an Android class, but just showing the logic here:

    ObservableBoolean a = new ObservableBoolean();
    ObservableBoolean b = new ObservableBoolean();

    public void call() {
        a.addOnPropertyChangedCallback(new OnPropertyChangedCallback() {
            @Override
            public void onPropertyChanged(android.databinding.Observable sender, int propertyId) {
                method2();
            }
        });
        b.addOnPropertyChangedCallback(new OnPropertyChangedCallback() {
            @Override
            public void onPropertyChanged(android.databinding.Observable sender, int propertyId) {
                //..you end the "chain" here
            }
        });
        method1();
    }

    void method1() {
        if(true) {
            a.set(true);
        }
        else {
            b.set(false);
        }
    }

    void method2() {
        if(true) {
            b.set(true);
        }
        else {
            b.set(false);
        }
    }
breakline
  • 5,776
  • 8
  • 45
  • 84
1

You define a class that holds an action (calling one of the methods) and with a corresponding failure handler (the else block of an if call)

public static class ActionWithFailureHandler {
    private Supplier<Boolean> action;
    private Runnable failureHandler;

    public ActionWithFailureHandler(Supplier<Boolean> action, Runnable failureHandler) {
        this.action = action;
        this.failureHandler = failureHandler;
    }
    //Getters for the instance variables

}

You make a list of the above and call each of the actions till one of the following happens

  1. One of the actions fails (i.,e one of the method returns false). In that case, you need to execute the failureHandler corresponding to that action.
  2. All actions pass. In this case, execute the successHandler (the logic that you execute when all methods return true).

private static void callAll(List<ActionWithFailureHandler> actionWithFailureHandlers, Runnable successHandler) {
    actionWithFailureHandlers.stream()
            .filter(actionWithFailureHandler -> !actionWithFailureHandler.getAction().get())
            .findFirst() //Find first failing action
            .map(ActionWithFailureHandler::getFailureHandler)
            .orElse(successHandler)
            .run(); //You might be running either the successHandler or the failureHandler for the first failed action
}

Driver code:

public static void main(String[] args) {
    Test test = new Test();
    List<ActionWithFailureHandler> actionWithFailureHandlers = com.google.common.collect.ImmutableList.of(
            new ActionWithFailureHandler(test::method1, () -> System.out.println("Method 1 returned false")),
            new ActionWithFailureHandler(test::method2, () -> System.out.println("Method 2 returned false")),
            new ActionWithFailureHandler(test::method3, () -> System.out.println("Method 3 returned false"))

        );
    callAll(actionWithFailureHandlers, () -> System.out.println("All returned true"));
}
Thiyagu
  • 17,362
  • 5
  • 42
  • 79
1

I use this technique - although some would find it odd.

boolean method1() {
    System.out.println("method1");
    return true;
}

boolean method2() {
    System.out.println("method2");
    return false;
}

boolean method3() {
    System.out.println("method3");
    return true;
}

void callAll() {
    boolean success = method1();
    success = success ? method2() : success;
    success = success ? method3() : success;
    if (success) {
        System.out.println("Success");
    } else {
        System.out.println("Failed");
    }
}
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
1

I could suggest you to use RX approach, with rxjava it should look like

public boolean test1() {
    Log.d("TESTIT", "test1 called");
    return true;
}

public boolean test2() {
    Log.d("TESTIT", "test2 called");
    return true;
}

public boolean test3() {
    Log.d("TESTIT", "test3 called");
    return false;
}

public boolean test4() {
    Log.d("TESTIT", "test4 called");
    return true;
}

public boolean elseMethod(boolean result) {
    if (result) return true;
    else {
        Log.d("TESTIT", "ELSE");
    }
    return false;
}

public void chainedCallback() {
    Observable.just(test1())
            .filter(this::elseMethod)
            .flatMap(aBoolean -> Observable.just(test2()))
            .filter(this::elseMethod)
            .flatMap(aBoolean -> Observable.just(test3()))
            .filter(this::elseMethod)
            .flatMap(aBoolean -> Observable.just(test4()))
            .filter(this::elseMethod)
            .subscribe();
}

call for chainedCallback() will print

test1 called
test2 called
test3 called
ELSE
dilix
  • 3,761
  • 3
  • 31
  • 55
-1

Exception firstly comes to my mind, but see the link below to learn more about its performance hit.

Original answer. I would do..

public class MyException extends Exception
{
}
public void doAll()
{
     try
     {
          method1();
          method2();
          method3();
     }catch (MyException e)
     {
            error();
      }
}

And let's assume that method1, method2, and method3 throws MyException when it fails.

Though it does not fit your question, it is a good pattern to use Exceptions.

public class Helper
{
    public Helper(Method m)
    {
          this.method=m;
     }
     public void Do() throws MyException
     {
          if(method.invoke()==false)
                throw new MyException ();
      }
} 

Using this class,

public void doAll()
    {
         Helper [] helpers={new Helper(this::method1), new Helper(this::method2), new Helper (this::method3)};

         try
         {
              for(Helper helper:helpers)
              {
                     helper.Do();
               }
         }catch (MyException e)
         {
                error();
          }
    }

But

according to the comment of @dilix and the link, it can be a performance-expensive strategy. So let's use them only for their purpose.

KYHSGeekCode
  • 1,068
  • 2
  • 12
  • 30
  • Exceptions are performance uneffective method of handling flows and designed for exceptional situationd and not for managing app flow. – dilix Aug 06 '18 at 10:19
  • @dilix OK I agree with that exceptions were originally built for exceptional situations. But what do you mean by performance ineffective? Could you explain more about it? – KYHSGeekCode Aug 06 '18 at 10:36
  • Check this https://blog.takipi.com/the-surprising-truth-of-java-exceptions-what-is-really-going-on-under-the-hood/ " Using exceptions for non exceptional purposes is bad ya’ll." (c) and it leads to "https://shipilev.net/blog/2014/exceptional-performance/". Main idea that exception is performance-expensive operation – dilix Aug 06 '18 at 10:40
  • @dilix Thanks so much for that useful link. – KYHSGeekCode Aug 06 '18 at 15:32