1

How can I refactor this long method of legacy Java code, loaded with side effects, into a more pure version?

public Result nonPureMethod(String param1, String param2){
  this.current_status = "running";
  String s1 = step1(param1, param2);
  this.logger.log("About to do step 2, this could take while");
  String s2 = step2(s1);
  this.logger.log("Completed step 2");
  String s3 = step3(s2);
  this.notifyOtherObject(s3);
  if (this.UserPressedEmergencyStop){ this.current_status = "stopped"; return; }
  String s4 = step4(s3);
  this.current_status = "completed";
  this.saveFile(s4);
  return new Result(s4);
}

In production, all of those side effects have to run. However sometimes I want to invoke a "pure" version of this method, which would look something like this:

public static Result pureMethod(String param1, String param2){
  String s1 = step1(param1, param2);
  String s2 = step2(s1);
  String s3 = step3(s2);
  String s4 = step4(s3);
  return new Result(s4);
}

Notes: I don't want to maintain two methods. If possible I'd like to have one. Also, I'd like to be able to optionally have some of the side effects sometimes, like logging, but not the others. What's the best way to refactor this code, so that I can call it and optionally have the side effects sometimes, and other times not?

I'm currently using Java 8, but I think this problem is pretty general. I've thought of two approaches so far to solve the problem. First, I could pass a boolean to the method: "runSideEffects". If false, just skip the code which runs the side effects. An alternative and more flexible solution would be to alter the function by requiring lambda functions passed as parameters, and calling them instead of invoking side effects. For example, a method like "void log(String msg)" could be passed as a parameter. The production invocation of the method can pass a function which will write the message to the logger. Other invocations can pass a method which effectively does nothing when log(msg) is called. Neither of these solutions feel great, which is why I'm asking the community for suggestions.

Dave
  • 2,735
  • 8
  • 40
  • 44
  • 3
    I don't see where anything new in java 8 would really help you with this method. What you suggest is roughly a [stragegy pattern](https://en.wikipedia.org/wiki/Strategy_pattern) where side effects vs no side effects = strategies. That is something you could have done all the time (and I can't imagine it would make your code better tbh) – zapl May 18 '16 at 23:29
  • 1
    Having two implementation is definitely a way to go (as suggested by @zapl). But you need it to maintain status and save to file right? – TriCore May 19 '16 at 03:12
  • Also, read the Effective Java item "Strive for Failure Atomicity." This has some advice about organizing code to minimize failures that could leave things in an inconsistent state. – Brian Goetz May 19 '16 at 14:52

5 Answers5

2

I’m not advertising this as a great solution, but more as a way to discuss the problems of your situation:

@SafeVarargs
public static Result pureMethod(
    String param1, String param2, Consumer<String>... optionalSteps) {
    if(optionalSteps.length>0) optionalSteps[0].accept(param1);
    String s1 = step1(param1, param2);
    if(optionalSteps.length>1) optionalSteps[1].accept(s1);
    String s2 = step2(s1);
    if(optionalSteps.length>2) optionalSteps[2].accept(s2);
    String s3 = step3(s2);
    if(optionalSteps.length>3) optionalSteps[3].accept(s3);
    String s4 = step4(s3);
    if(optionalSteps.length>4) optionalSteps[4].accept(s4);
    return new Result(s4);
}
public Result nonPureMethod(String param1, String param2) {
  return pureMethod(param1, param2, 
      arg -> this.current_status = "running",
      arg -> this.logger.log("About to do step 2, this could take while"),
      arg -> this.logger.log("Completed step 2"),
      s3  -> { this.notifyOtherObject(s3);
            if (this.UserPressedEmergencyStop) {
                this.current_status = "stopped";
                throw new RuntimeException("stopped");
            }
        },
      s4  -> { this.current_status = "completed"; this.saveFile(s4); });
}

What’s striking here is how little these optional actions have in common. The code snippet above accepts that some actions won’t use the provided argument and for the first action, one of the two parameters has been chosen arbitrarily. The alternative would be to use a BiConsumer, requiring all other actions to carry an unused parameter. But the worst offender here is the termination via exception in the fourth action. A clean solution would be to use a function type returning a boolean to determine whether to continue but that would force all actions to return a boolean as well, e.g. turn a simple lambda expression like arg -> this.current_status = "running" into arg -> { this.current_status = "running"; return true; }, etc.

As a side note, I don’t know which logging framework you are using, but logging normally is already an action that can be turned into side effect free mode via configuration options.

Maybe it helps to categorize your actions and create different parameters, e.g. a Logger, a status updater and an early termination predicate, e.g.

public static Result pureMethod(String param1, String param2,
        Logger logger, ObjIntConsumer<String> statusUpdater, IntPredicate cont) {
    statusUpdater.accept(null, 0);
    String s1 = step1(param1, param2);
    statusUpdater.accept(s1, 1);
    if(!cont.test(1)) return null;
    logger.log("About to do step 2, this could take while");
    String s2 = step2(s1);
    statusUpdater.accept(s2, 2);
    if(!cont.test(2)) return null;
    logger.log("Completed step 2");
    String s3 = step3(s2);
    statusUpdater.accept(s3, 3);
    if(!cont.test(3)) return null;
    String s4 = step4(s3);
    statusUpdater.accept(s4, 4);
    return new Result(s4);
}
public static Result pureMethod(String param1, String param2) {
    Logger logger=Logger.getAnonymousLogger();
    logger.setLevel(Level.OFF);
    return pureMethod(param1, param2, logger, (s,i)->{}, i->true);
}
public Result nonPureMethod(String param1, String param2) {
    return pureMethod(param1, param2, this.logger,
        (s,i)-> { switch (i) {
            case 0: this.current_status = "running"; break;
            case 3: this.notifyOtherObject(s); break;
            case 4: this.current_status = "completed"; this.saveFile(s); break;
        }}, i -> {
            if(i==3 && this.UserPressedEmergencyStop) {
                this.current_status = "stopped";
                return false;
            }
            else return true;
        });
}

but it’s still tight to the use case of nonPureMethod in some ways…

Holger
  • 285,553
  • 42
  • 434
  • 765
1

One option is to extract the method into a class with an empty template method for each step, and override it for the non-pure version:

class Method {
    void beforeStart() {};
    void afterStep1(String result) {};
    void afterStep2(String result) {};
    void afterStep3(String result) {};
    void afterStep4(String result) {};

    final Result execute(String param1, String param2) {
        beforeStart();
        String s1 = step1(param1, param2);
        afterStep1(s1);
        String s2 = step2(s1);
        afterStep2(s2);
        String s3 = step3(s2);
        afterStep3(s3);
        String s4 = step4(s3);
        afterStep4(s4);
        return new Result(s4);
    }
}

Then you could define one or more subclasses that override the provided methods to insert side-effects.

shmosel
  • 49,289
  • 6
  • 73
  • 138
0

Pass functions as parameters. Make the functions do the side effects. You can simply not pass the side-effect functions as parameters if you want to invoke a "pure" version of the function.

I now have this available in different languages as a Github repository: https://github.com/daveroberts/sideeffects

package foo;

import java.util.function.BiConsumer;
import java.util.function.BooleanSupplier;
import java.util.function.Consumer;

public class SideEffects{
  public static void main(String args[]){
    System.out.println("Calling logic as a pure function");
    String result = logic("param1", "param2", null, null, null, null, null);
    System.out.println("Result is "+result);
    System.out.println();

    System.out.println("Calling logic as a regular function");
    result = logic("param1", "param2",
        (level,msg)->{System.out.println("LOG ["+level+"]["+msg+"]");},
        (status)->{System.out.println("Current status set to: "+status); },
        (obj)->{System.out.println("Called notify message on object: "+obj.toString());},
        ()->{boolean dbLookupResult = false; return dbLookupResult;},
        (info)->{System.out.println("Info written to file [["+info+"]]");}
        );
    System.out.println("Result is "+result);
  }

  public static String logic(String param1, String param2,
      BiConsumer<String, String> log,
      Consumer<String> setStatus,
      Consumer<Object> notify,
      BooleanSupplier eStop,
      Consumer<String> saveFile){
  if (setStatus != null){ setStatus.accept("running"); }
  String s1 = param1+"::"+param2;
  if (log != null){ log.accept("INFO", "About to do Step 2, this could take awhile"); }
  String s2 = s1+"::step2";
  if (log != null){ log.accept("INFO", "Completed step 2"); }
  String s3 = s2+"::step3";
  if (notify != null) { notify.accept("randomobjectnotify"); }
  if (eStop != null && eStop.getAsBoolean()){
    if (setStatus != null){ setStatus.accept("stopped"); }
    return "stoppedresult";
  }
  String s4 = s3+"::step4";
  if (setStatus != null){ setStatus.accept("completed"); }
  if (saveFile!= null){ saveFile.accept("Logic completed for params "+param1+"::"+param2); }
  return s4;
  }
}
Dave
  • 2,735
  • 8
  • 40
  • 44
-1

I believe you can rather than make it do the steps separately put them into one using a resulting variable like so `

public int Steps(int param1,int param2){
//whatever you want your first step to do make result into a variable
int param3 = param1-param2;

//Same with step 2 ,3 and so on
int param4 = param3*param1;

}` 
Azamp19
  • 1
  • 5
  • or any object you want not just ints. – Azamp19 May 18 '16 at 23:17
  • I don't quite follow this answer. I'm not sure what you mean by subtracting param 1 from param 2, and what multiplying param 3 by param 1 would do. The steps are left vague here, but they are composed of many lines of code. What I wanted to show was that in between those lines, there are other lines which alter state, but I don't want them to do so sometimes when invoking the method. – Dave May 18 '16 at 23:58
  • 1
    Not sure, how this has anything to do with the question. – TriCore May 19 '16 at 02:59
  • It was basically saying rather than make the steps separate methods put them together. The equations are irrelevant. – Azamp19 May 19 '16 at 03:51
-1

It is possible, but a bit werid somehow.

public class MethodPipeline<T, I, R> {
    private final MethodPipeline<T, ?, I> prev;
    private final int kind;
    private final Function<? extends I, ? extends R> f;
    private final Runnable r;
    private final Consumer<? extends R> c;
    private MethodPipeline(Function<? extends I, ? extends R> l, MethodPipeline<? extends T, ?, ? extends I> prev) {
        kind = 0;
        f = l;
        r = null;
        c = null;
        this.prev = prev;
    }
    private MethodPipeline(Runnable l, MethodPipeline<? extends T, ?, ? extends I> prev) {
        kind = 1;
        f = null;
        r = l;
        c = null;
        this.prev = prev;
    }
    private MethodPipeline(Consumer<? extends R> l, MethodPipeline<? extends T, ?, ? extends I> prev) {
        kind = 2;
        f = null;
        r = null;
        c = l;
        this.prev = prev;
    }
    //...various public consructor
    public <R1> MethodPipeline<T, R, R1> then(Function<? extends R, ? extends R1> convertor) {
        return new MethodPipeline<>(convertor, this);
    }
    public MethodPipeline<T, I, R> sideEffect(Runnable sideEffect) {
        return new MethodPipeline<>(sideEffect, this);
    }
    public MethodPipeline<T, I, R> sideEffect(Consumer<? extnds R> sideEffect) {
        return new MethodPipeline<>( sideEffect, this);
    }
    public R run(T param, boolean sideEffect) {
        I v = prev.run(param);
        switch (kind) {
        case 0:
            return f.apply(v);
        case 1:
            if (sideEffect)
                r.run();
            return v;
        case 2:
            if (sideEffect)
                c.accept(v);
            return v;
        }
    }
}

I designed it a pipeline, just like j.u.stream do. The run is recursive for the sake of type safety. Use it with caution: don't put too much work into the pipeline. It may result in a StackOverFlowException.

PS: Web written. Not tested. Not even compiled for once. Use at your own risk. Bounded type variables may need some refactor, change it yourself.

glee8e
  • 6,180
  • 4
  • 31
  • 51