Avoid continuous "if (...)" checks while executing function

Question

I have a function which looks like the following:

public Status execute() {
    
    Status status = doSomething();

    if (status != Status.ABORTED) {
        status = doSomethingElse();
    }

    if (status != Status.ABORTED) {
        status = doAgainSomethingElse(param1, param2);
    }

    if (status != Status.ABORTED) {
        doSomethingWhichDoesntReturn(param3);
    }

    //etc.
    
    return status;
}

So basically this function needs to return a Status. This is computed by a first function, and then recomputed by successive functions at the condition that, when those functions are executed, status != Status.ABORTED.

I would like to refactor this code but I don't have any valid idea in my mind.

If it was always status = someFunction(someParam), I would have used a list of Function<TypeInput, Status> and executed that list in loop:

List<Function<TypeInput, Status>> actions = List.of(function1, function2...);
for (Function<TypeInput, Status> f : actions) {
    if (status != Status.ABORTED) {
        status = f.apply(input);
    }
}

The problem though is that each action may be different (sometimes it's a function which returns Status, sometimes there are parameters but not always the same size, sometimes it's just a void function etc.)

Does anyone have any idea?

Note: as soon as the status gets Status.ABORTED, I can return (I don't need to execute the rest of the function as anything is executed only if the status is not Status.ABORTED).

Answer 1

Expanding on dave's idea (I was thinking along the same lines) you could provide a class that represents something like a conditional chain:

//individual "chain links", i.e. elements in the chain
interface ChainLink<V> {
    V execute(V v) throws Exception;
}

class ConditionalChain<V> {
    private final V initialValue;
    private final Predicate<V> condition;
    private final Collection<ChainLink<V>> links = new LinkedList<>();
    
    //creates the chain with the initial condition value and the condition
    public ConditionalChain(V initialValue, Predicate<V> condition) {
        this.initialValue = initialValue;
        this.condition = condition;
    }

    //execute the chain
    public V execute() throws Exception {
        V v = initialValue;
        
        for( ChainLink<V> link : links ) {
          //apply the condition first to test the initial value
          if( !condition.test(v) ) {
            break;
          }

          v = link.execute(v);                
        }
        
        return v;
    }
    
    //chain a Callable that returns a new value
    public ConditionalChain<V> chain(Callable<V> c) {
        links .add(v -> c.call() );
        return this;
    }
    
    //chain a Runnable that doesn't change the value
    ConditionalChainer<V> chain(Runnable r) {
        links .add(v -> { r.run(); return v; } );
        return this;
    }
    
    //helper to get the chain started
    public static <T>  ConditionalChain<T> start(T initialValue, Predicate<T> condition) {
        return new ConditionalChain<T>(initialValue, condition);
    }
}

We're using out own (internal) functional interface to allow for returning the status even when using a runnable and to support exceptions being thrown.

This could also be expanded to allow for functions that take the current status as a parameter.

The chain itself could then look like this:

 Status status = ConditionalChain.start(Status.RUNNING, s -> s != Status.ABORTED )
        .chain(() -> doSomething())
        .chain(() -> doSomethingElse())
        .chain(() -> doSomethingWhichDoesntReturn(param3))
        .chain(() -> doAgainSomethingElse("param1", "param2"))
        .execute();

That way you'd be able to reuse the chain with different predicates. You could even return a "chain result" that contains the status as well as the index of the last element that has been executed, e.g. if you're interested that execution stopped after doSomethingElse().

Answer 2

This looks like a good case for a try-catch approach. You could throw an exception in either of the methods e.g. StatusAbortedException and catch that to return the appropriate Status. It could look like this

try {
 Status status = doSomethingElse();
 status = doAgainSomethingElse(param1, param2);
 status = doSomethingWhichDoesntReturn(param3); // this one probably does smth else
 return status;
} catch (StatusAbortedException e){
  // return Status.Aborted 
}

Answer 3

There are several options you can go for. One option is a continuation passing style. This doesn't look all that good in Java, but you can do something similar.

// This is pseudo code, intended to illustrate the concept.
cpsMethod(Arg... args, ClosureOverFunctionSoItIsNullary continuation) {
 // do stuff
 continuation.call();
}

So basically, the method gets what's supposed to happen next passed into it. There are some downsides to this approach in Java, namely that you don't have tail-call optimization, so you can get a stack overflow, and perhaps more importantly, it looks very different from normal Java.

// Illustrative pseudo code
return doSomething(() -> doSomethingElse(() -> doAgainSomethingElse(param1, param2, () -> doSomethingWhichDoesntReturn())));

This removes the ifs, or rather, put the test inside every method, which now has to decide if it's going to continue, or if it's going to just return Status.ABORTED. You could of course make this thing prettier by putting the handling outside and just take the methods as producers, give in a Predicate/hardcode the test, and just offer varargs:

private continuationPasser(Supplier<Status> first, Supplier<Status>... rest) {
    Objects.requireNonNull(first);
    Status status = first.get();
    for(Supplier<T> continuation : methods) {
        status = continuation.get();
        if(status == Status.ABORTED) {
            return status;
        }
    }
}

Dirt simple code, does exactly what you'd expect, and now your call on top will go from:

public Status execute() {
    
    Status status = doSomething();

    if (status != Status.ABORTED) {
        status = doSomethingElse();
    }

    if (status != Status.ABORTED) {
        status = doAgainSomethingElse(param1, param2);
    }

    if (status != Status.ABORTED) {
        doSomethingWhichDoesntReturn(param3);
    }

    //etc.
    
    return status;
}

To something like:

public Status execute() {
    return continuationPasser(
      this::doSomething,
      this::doSomethingElse,
      () -> doAgainSomethingElse(arg1, arg2);
      () -> doSomethingWhichDoesntReturn(arg3));

Except for, you know, the last one doesn't return anything. If it's trivial to make it return something, then you could just do that. If that's not trivial, you can just change the type from a Supplier to a Function<Status, T>, and you can pass in the last status if you want.

But that's an option. Take a functional idea and make it work. This has the benefit of being very clear if you know what continuation passing is. You could generalize the idea to take in a predicate too, if you'd like. Another way to do this would be to change the continuationPasser a bit, to make it pass in the previous result, and let the methods themselves decide what they want to do.

Then continutationPasser can look like this:

continuationPasser(Function<Status, Status> first, Function<Status, Status>... rest) {
    Objects.requireNonNull(first);
    Status status = first.apply(Status.SOME_REASONABLE_VALUE_LIKE_NOT_STARTED);
    // You could use some reduce function here if you want to.
    // The choice of a loop here is just my personal preference.
    for(Function<Status, Status> fun : rest) {
      status = rest.apply(status);
    }
    return status;
}

This makes the continuation passer even simple. You start off by applying the first function, to get a starting value. Then you just for-each over the rest of them. And they can just start with checking for the ABORTED status and exit early. You'll still have the ifs, but your main running code will look positively neat now. You can always wrap your methods in something like:

Function<Status, Status> runIfNotAborted(Supplier<Status> supplier) {
  return (Status s) -> s == ABORTED? ABORTED : supplier.get();
}

Function<Status, Status> returnPreviousStatus(Runnable code) {
  return (s) -> {
    code.run();
    return s;
  }
}

And now you don't even have to change your methods. (But if you were to do this style that might be a better option if it was available.)

public Status execute() {
    return continuationPasser(
      runIfNotAborted(this::doSomething),
      runIfNotAborted(this::doSomethingElse),
      runIfNotAborted(() -> doAgainSomethingElse(arg1, arg2)),
      runIfNotAborted(returnPreviousStatus(() -> doSomethingWhichDoesntReturn(arg3)));

And now it's quite clear what's going on. We're building functions on top of functions, in what looks a bit like a functional decorator-pattern.

This is a very general idea, and you can do this more specialized or generalize it more if you want to. But be careful or you'll write a framework to not have to write an if/else. Jenkins uses this idea for its pipelines, but has a bit more stuff in it to pass along the environment as well for example.

Answer 4

You could have a closure over the different function signatures, so they all have the same signature, and then iterate over your list like you wanted until the status has changed. Something like this (but using a list, I was just a little lazy since this gets the point across):

https://onlinegdb.com/4JB1flbww

interface StatusInterface {
  public String fixStatus();
}

public class Main
{
  public static String A(boolean x) {
      if (x) {
          return "fixed";
      }
      return "broken";
  }

  public static String B(boolean x, boolean y) {
      if (!x && y) {
          return "fixed";
      }
      return "broken";
  }
public static void main(String[] args) {
    // Lambda Expression
    boolean x = false;
    boolean y = true;
      StatusInterface AWrapped = () ->
      {
          return A(x);
      };
      StatusInterface BWrapped = () ->
      {
          return B(x, y);
      };

      // Calling the above interface
      String status = "broken";
      for (int i = 0; i < 2 && status.equals("broken"); i++) {
          if (i == 0) status = AWrapped.fixStatus();
          else status = BWrapped.fixStatus();
      }
  System.out.println(status);
}