Trying to do a minor refactor using lambdas in Java

Question

I am working on part of a program (concerning speech recognition and a remote control car), where the code transmit(XXXXX); disableAutoMode(); is repeated many times. For curiosity's sake I would like to convert this into a lambda function similar to this var f = p -> transmit(p); disableAutoMode(); (forgive the var; I don't know what the type of the expression would be) and then call it in a way similar to this: f("s");, f("a"); and f("f"); or something similar to f.call("s");, f.call("a"); and f.call("f");.

What is the correct syntax to use a simple lambda function in Java similar to how I described above? (What should I put down as it's type instead of saying var?)

Here is the block of code, if you're curious:

@Override
public void onResult(Hypothesis hypothesis) {
    if (hypothesis != null) {
        String text = hypothesis.getHypstr();
        Log.i("onresult",text);
        ToastMaster(text);

        switch (text) {
            case "forward":
            case "go forward":
                transmit("f");
                disableAutoMode();
                break;
            case "go back":
            case "go backward":
            case "back":
            case "backward":
            case "reverse":
                transmit("b");
                disableAutoMode();
                break;
            case "skid left":
            case "go left":
                transmit("l");
                disableAutoMode();
                break;
            case "skid right":
            case "go right":
                transmit("r");
                disableAutoMode();
                break;
            case "turn left":
                transmit("q");
                disableAutoMode();
                break;
            case "turn right":
                transmit("e");
                disableAutoMode();
                break;
            case "reverse left":
                transmit("z");
                disableAutoMode();
                break;
            case "reverse right":
                transmit("x");
                disableAutoMode();
                break;
            case "stop":
                disableAutoMode();
                break;
            case "automatic":
                toggleAutoMode(null);
                break;
            case "enable automatic mode":
                enableAutoMode();
                break;
            case "disable automatic mode":
                disableAutoMode();
                break;
        }
    }
}

score 8 · Answer 1 · answered Feb 24 '16 at 14:51

A more ambitious refactoring would build on the "turn code into data" principle that lambdas are all about, turning your switch statement from code to data as well. How about:

// One-time setup of the machine
Map<String, Consumer<String>> actions = new HashMap<>();
actions.put("go forward", x -> { transmit(x); disableAutoMode(); });
actions.put(...)
...

public void onResult(Hypothesis hypothesis) {
    if (hypothesis != null) {
        String text = hypothesis.getHypstr();
        Log.i("onresult",text);
        ToastMaster(text);

        Consumer<String> action = actions.get(text);
        if (action != null)
            action.accept(text);
    }
}

Btw, what do you think about `actions.getOrDefault(text, x -> {}).accept(text)`? Is it reasonable? — Tagir Valeev, Feb 24 '16 at 16:25
Sure, that's fine too. The high order bit was, "turn this big bag of code into a bag of data". — Brian Goetz, Feb 24 '16 at 16:27

Michael Lloyd Lee mlk · Accepted Answer · 2016-02-24T13:27:33.153

4

In this case you want a Consumer.

Consumer<String> function = (x) -> { transmit(x); disableAutoMode(); };
function.accept("hello!");

However I'm not sure why you want to use a lambda expression here, you could just create a plain old method and call that.

If you are after a more meaningful refactor one option would be to switch to a map of String, Action/Runnable. While you would end up with more code, the goal of refactors is not to "make it smaller" but to make it more readable/maintainable. By splitting each action into its own small self-contained class each action can be tested in isolation with minimal set-up. Each action can be reused (as it is just a class). With a good naming strategy it would be obvious to readers what is going on without having to dig through a large switch statement.

edited Feb 24 '16 at 13:27

answered Feb 24 '16 at 11:12

Michael Lloyd Lee mlk

14,561
3
44
81

The win from lambdas comes not locally, but by turning the `switch` into something more data driven -- see http://stackoverflow.com/a/35605401/3553087 – Brian Goetz Feb 25 '16 at 16:10
You have recommended this same as the second part of my post. – Michael Lloyd Lee mlk Feb 25 '16 at 16:17

score 2 · Answer 3 · answered Feb 24 '16 at 11:10

Calling the lambda would be more tedious than using a simple helper method:

private void m(String x) {
    transmit(x);
    disableAutoMode();
}

An (more verbose) alternative would be using lambdas:

Consumer<String> consumer = (x) -> {
    transmit(x);
    disableAutoMode();
};

and then calling it similar to

consumer.accept("f");

score 0 · Answer 4 · answered Feb 24 '16 at 11:16

What you want to do, is indeed possible with a lambda expression, but the saving won’t be so big:

@Override
public void onResult(Hypothesis hypothesis) {
    if (hypothesis != null) {
        String text = hypothesis.getHypstr();
        Log.i("onresult",text);
        ToastMaster(text);

        Consumer<String> transmitDisable=s->{ transmit(s); disableAutoMode(); };
        switch (text) {
            case "forward": case "go forward":
                transmitDisable.accept("f");
                break;
            case "go back":  case "go backward": case "back":
            case "backward": case "reverse":
                transmitDisable.accept("b");
                break;
            case "skid left": case "go left":
                transmitDisable.accept("l");
                break;
            case "skid right": case "go right":
                transmitDisable.accept("r");
                break;
            case "turn left":     transmitDisable.accept("q"); break;
            case "turn right":    transmitDisable.accept("e"); break;
            case "reverse left":  transmitDisable.accept("z"); break;
            case "reverse right": transmitDisable.accept("x"); break;
            case "stop": disableAutoMode(); break;
            case "automatic": toggleAutoMode(null); break;
            case "enable automatic mode": enableAutoMode(); break;
            case "disable automatic mode": disableAutoMode(); break;
        }
    }
}

But you can remove the code duplication without lambda expressions as well, if you consider the not-so-commonly-used code flow control constructs of Java:

@Override
public void onResult(Hypothesis hypothesis) {
    if (hypothesis != null) {
        String text = hypothesis.getHypstr();
        Log.i("onresult",text);
        ToastMaster(text);

        transmitAndDisable: {
            final String toTransmit;
            switch (text) {
                case "forward":    case "go forward": toTransmit="f"; break;
                case "go back":    case "go backward": case "back":
                case "backward":   case "reverse":  toTransmit="b"; break;
                case "skid left":  case "go left":  toTransmit="l"; break;
                case "skid right": case "go right": toTransmit="r"; break;
                case "turn left":     toTransmit="q"; break;
                case "turn right":    toTransmit="e"; break;
                case "reverse left":  toTransmit="z"; break;
                case "reverse right": toTransmit="x"; break;

                case "stop": disableAutoMode(); break transmitAndDisable;
                case "automatic": toggleAutoMode(null); break transmitAndDisable;
                case "enable automatic mode": enableAutoMode(); break transmitAndDisable;
                case "disable automatic mode": disableAutoMode(); break transmitAndDisable;
                default: break transmitAndDisable;
            }
            transmit(toTransmit);
            disableAutoMode();
        }
    }
}

The structure might be not so easy to read, but note how declaring the toTransmit as final helps greatly. Because it’s final, it can’t be initialized with a fall-back value, which implies that each alternative getting to the shared transmit(…); disableAutoMode(); code must have initialized the variable exactly once.

In other words,

if you forget to specify the transmitAndDisable label at a break of a case that is not intended to call the shared code, the compiler will immediately yell because toTransmit has not been initialized.
if you forget a break between one of the alternatives intended to invoke the shared code, the compiler will immediately reject the code because it’s assigning toTransmit twice
if you mistakenly specify the transmitAndDisable label at a break of a case that is intended to call the shared code, good compilers will issue a warning about an unused assignment

Personally I'd not do the second option here. I think it significantly hurts readability and does not save much compared to a plain old method. — Michael Lloyd Lee mlk, Feb 24 '16 at 11:19
@mlk: a plain old method is a viable option in this case and it’s definitely the preferred option if you have multiple common cases instead of a single dominant case. Note that the common case here is actually a *map* from the `case` string to the string passed to `transmit`. Doing the mapping using an actual `Map` allows to treat all common cases equally right inside the `switch` rather than in separate code, be it in a method or surrounding block… — Holger, Feb 24 '16 at 11:32
@mlk: by the way, as noted, the second option provides a guaranty that you can’t forget a `break` for one of the common cases, which delegating to a method does not offer. — Holger, Feb 24 '16 at 11:36
Honestly, aside from the guarantees regarding the `final toTransmit` variable and the `switch` construct, there's no way for the second option to be better than almost anything... It's hardly readable, depends on a kind of recursive declaration of the `switch` statement and dangerously resembles 70s or 80s code... — fps, Feb 24 '16 at 12:37
@Federico Peralta Schaffner: well, I accept this opinion, but if you think this over *consequently*, you should apply this to `switch` statements in general as everything you may say against labelled blocks applies to `switch` as well. That’s an acceptable point of view, but since this question was about a `switch` statement in the first place and both, `switch` and labelled statements are part of the Java language, I think my answer is valid for this specific question. — Holger, Feb 24 '16 at 14:07
@Holger I always use `if` and `else if` and `else`. :) Anyways, I agree in that this is valid java code for this specific question. However, I think that you won't find many supporters to fight this labels battle... — fps, Feb 24 '16 at 17:55
+1 for teaching me something new about Java syntax. I've never seen those "labelled blocks and breaks" before — TheIronKnuckle, Feb 25 '16 at 06:18

Trying to do a minor refactor using lambdas in Java

4 Answers4