Hallo zusammen,
am 19.03.2015 waren Hans Peter Bornhauser und ich Guestspeaker beim Microsoft Techtalk in Wallisellen. Dass Ereignis war mit mehr als 100 Leuten komplett ausverkauft und drehte sich innerhalb von 3 Vorträgen rund um das Thema Asp.Net und Azure.
Recently I got the task to change the behaviour of some legacy Java code to make it work for new hardware. The code was first written in the era of Java 1.4 and naturally was quite verbose and, worse, contained a lot of duplication. Luckily the new version of the application is targeted to run on an shiny new Java 8 VM. Following the boyscout rule I decided to refactor the code in order to remove the duplication and make it more readable. As an added benefit, this would also improve its reusability. Since I’m a fan of Functional Javascript and Java 8 now supports the functional style, I went to see if I could use any of this.
This is the code we start with:
package com.noser.blog.functionaljava8;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
public class App {
private Sensor off;
private Sensor stateA;
private Sensor stateB;
private Sensor stateC;
private Sensor stateD;
private ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
private StateMachine stateMachine;
public App(SensorFactory factory, StateMachine stateMachine) {
this.stateMachine = stateMachine;
// initialize sensors
off = factory.findSensor("off_sensor");
stateA = factory.findSensor("state_a_sensor");
stateB = factory.findSensor("state_b_sensor");
stateC = factory.findSensor("state_c_sensor");
stateD = factory.findSensor("state_d_sensor");
executor.scheduleAtFixedRate(new Looper(), 0L, 100L, TimeUnit.MILLISECONDS);
}
private class Looper implements Runnable {
private boolean offGuard, stateAGuard, stateBGuard, stateCGuard, stateDGuard;
public void run() {
if (off.read()) {
if (!offGuard) {
stateMachine.setState(State.OFF);
offGuard = true;
}
} else {
offGuard = false;
}
if (stateA.read()) {
if (!stateAGuard) {
stateMachine.setState(State.A_1);
stateAGuard = true;
}
} else {
stateAGuard = false;
}
if (stateB.read()) {
if (!stateBGuard) {
stateMachine.setState(State.B);
stateBGuard = true;
}
} else {
stateBGuard = false;
}
if (stateC.read()) {
if (!stateCGuard) {
if (stateMachine.getState() == State.C_1) {
stateMachine.setState(State.C_2);
} else {
stateMachine.setState(State.C_1);
}
stateAGuard = true;
}
} else {
stateAGuard = false;
}
if (stateD.read()) {
if (!stateDGuard) {
stateMachine.setState(State.D);
stateDGuard = true;
}
} else {
stateDGuard = false;
}
}
}
}
And here we have the Sensor interface and State classes (the naming here is for educational purposes only, don’t name your states or classes like this at home):
package com.noser.blog.functionaljava8;
public interface Sensor {
boolean read();
}
package com.noser.blog.functionaljava8;
public enum State {
OFF,
A_1,
A_2,
A_3,
B,
C_1,
C_2,
D;
}
As you can see on the lines 38-86 of the App class, the run() method of the Looper class has lots of code duplication in it. The same steps of reading the sensor, checking the guard flag, setting the desired state and updating the guard flag are repeated for every new sensor-state combination.
Fortunately we have an interface so that we can use the Decorator pattern to take care of that.
package com.noser.blog.functionaljava8;
public class GuardedSensor implements Sensor {
private boolean guard;
private Sensor sensor;
public GuardedSensor(Sensor sensor) {
this.sensor = sensor;
}
public boolean read() {
if (sensor.read()) {
if (!guard) {
guard = true;
return true;
}
} else {
guard = false;
}
return false;
}
}
With this we can update the App class to remove the duplication.
package com.noser.blog.functionaljava8;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
public class AppRefactored {
private Sensor off;
private Sensor stateA;
private Sensor stateB;
private Sensor stateC;
private Sensor stateD;
private ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
private StateMachine stateMachine;
public AppRefactored(SensorFactory factory, StateMachine stateMachine) {
this.stateMachine = stateMachine;
// initialize sensors
off = new GuardedSensor(factory.findSensor("off_sensor"));
stateA = new GuardedSensor(factory.findSensor("state_a_sensor"));
stateB = new GuardedSensor(factory.findSensor("state_b_sensor"));
stateC = new GuardedSensor(factory.findSensor("state_c_sensor"));
stateD = new GuardedSensor(factory.findSensor("state_d_sensor"));
executor.scheduleAtFixedRate(new Looper(), 0L, 100L, TimeUnit.MILLISECONDS);
}
private class Looper implements Runnable {
public void run() {
if (off.read()) {
stateMachine.setState(State.OFF);
}
if (stateA.read()) {
stateMachine.setState(State.A_1);
}
if (stateB.read()) {
stateMachine.setState(State.B);
}
if (stateC.read()) {
if (stateMachine.getState() == State.C_1) {
stateMachine.setState(State.C_2);
} else {
stateMachine.setState(State.C_1);
}
}
if (stateD.read()) {
stateMachine.setState(State.D);
}
}
}
}
This is much better. On lines 24 to 27 of AppRefactored we decorate the sensors with GuardedSensor and can then on lines 36 to 59 get rid multiple ugly guard checkings. The resulting code is much easier to read and its purpose is more evident. This facilitates finding errors in the domain logic.
The new requirement was, that it should be possible to put the state machine into OFF state by again firing the respective sensor once the machine is in the selected state. For example, given the machine is in state B, then firing the sensor "state_b_sensor" it should switch to state OFF. But if the machine is in any other state, then it should switch to B. Since states A and C are special, they will need some special treatment as well. For A the machine should switch to OFF for any of the “substates” of A the machine was in (so for example A_1 and A_2 would both go to OFF). State C should only go to OFF for C_2 and keep its behaviour in C_1 (so still go from C_1 to C_2).
The non-functional solution is pretty straightforward: in order to find out what should be the new state, we have to check the current state (stateMachine.getState()) first and then decide.
package com.noser.blog.functionaljava8;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
public class NonFunctionalApp {
private Sensor off;
private Sensor stateA;
private Sensor stateB;
private Sensor stateC;
private Sensor stateD;
private ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
private StateMachine stateMachine;
public NonFunctionalApp(SensorFactory factory, StateMachine stateMachine) {
this.stateMachine = stateMachine;
// initialize sensors
off = new GuardedSensor(factory.findSensor("off_sensor"));
stateA = new GuardedSensor(factory.findSensor("state_a_sensor"));
stateB = new GuardedSensor(factory.findSensor("state_b_sensor"));
stateC = new GuardedSensor(factory.findSensor("state_c_sensor"));
stateD = new GuardedSensor(factory.findSensor("state_d_sensor"));
executor.scheduleAtFixedRate(new Looper(), 0L, 100L, TimeUnit.MILLISECONDS);
}
private class Looper implements Runnable {
public void run() {
if (off.read()) {
stateMachine.setState(State.OFF);
}
if (stateA.read()) {
stateMachine.setState(newState(stateMachine.getState().isA(), State.A_1));
}
if (stateB.read()) {
stateMachine.setState(newState(stateMachine.getState() == State.B, State.B));
}
if (stateC.read()) {
if (stateMachine.getState() == State.C_1) {
stateMachine.setState(State.C_2);
} else {
stateMachine.setState(newState(stateMachine.getState() == State.C_2, State.C_1));
}
}
if (stateD.read()) {
stateMachine.setState(newState(stateMachine.getState() == State.D, State.D));
}
}
}
private State newState(boolean offCheck, State state) {
return offCheck ? State.OFF : state;
}
}
To make the check for the A states more convenient, we implemented a short method in State.
package com.noser.blog.functionaljava8;
public enum State {
OFF,
A_1,
A_2,
A_3,
B,
C_1,
C_2,
D;
boolean isA() {
switch (this) {
case A_1:
case A_2:
case A_3:
return true;
default:
return false;
}
}
}
The problem with this non-functional solution is, that it’s ugly. We can see, for example on line 45, that it’s obscuring the domain logic a bit. First there’s the lengthy check on the current state of the machine and then there’s call to newState which was introduced to tackle the verbosity of the Java code but doesn’t really help much to improve readability. Of course we could rename it to something like stateUnless but, without named parameters this is not much better either.
To make the code clearer I wanted to have something like the following:
stateMachine.setState(current(isSomeCondition()) ? State.OFF : State.B);
Creating the current() method is easy enough. We need a method that takes something that works on a state and returns a boolean. Sounds like a job for a Predicate:
private boolean current(Predicatepred) { return pred.test(stateMachine.getState()); }
But creating the Predicate is trickier. For the substates of A it’s easy, because we have the isA() method defined in State. But for B, C, and D we have to check using the == operator or the equals() method of Object. But these take two arguments instead of one. So the obvious thing is to implement a Predicate for every state, like so:
package com.noser.blog.functionaljava8;
public enum State {
OFF,
A_1,
A_2,
A_3,
B,
C_1,
C_2,
D;
boolean isA() {
switch (this) {
case A_1:
case A_2:
case A_3:
return true;
default:
return false;
}
}
boolean isB() {
return this == B;
}
boolean isC1() {
return this == C_1;
}
boolean isC2() {
return this == C_2;
}
boolean isD() {
return this == D;
}
}
So then we can do this:
stateMachine.setState(current(State::isB) ? State.OFF : State.B);
Unfortunately we have just reintroduced code duplication. This time in the State class. Furthermore, the code I had to work with has a lot more states, making this solution very tedious.
What we need, is a way to transform the BiPredicate, that is the equals() method, into a Predicate on the fly by fixing one of its "arguments" on the state we wish to test. This technique is called Partial Application. Unfortunately this is not directly supported by Java, as it is in Javascript for example. But implementing it is easy enough:
package com.noser.blog.functionaljava8;
import java.util.function.BiPredicate;
import java.util.function.Predicate;
public final class FunctionalTools {
public static Predicate partial(BiPredicate bipred, U fixed) {
return (T t) -> bipred.test(t, fixed);
}
}
With this we can get rid of our code duplication by creating a static factory method is() in the State class
package com.noser.blog.functionaljava8;
import java.util.function.Predicate;
public enum State {
OFF,
A_1,
A_2,
A_3,
B,
C_1,
C_2,
D;
boolean isA() {
switch (this) {
case A_1:
case A_2:
case A_3:
return true;
default:
return false;
}
}
static Predicate is(State state) {
return FunctionalTools.partial(State::equals, state);
}
}
and finally cleanup the App class to make the domain logic evident again. To make the code even clearer, I did a static import of the State class and renamed the current method to currentState.
package com.noser.blog.functionaljava8;
import static com.noser.blog.functionaljava8.State.*;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
public class FunctionalApp {
private Sensor off;
private Sensor stateA;
private Sensor stateB;
private Sensor stateC;
private Sensor stateD;
private ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
private StateMachine stateMachine;
public FunctionalApp(SensorFactory factory, StateMachine stateMachine) {
this.stateMachine = stateMachine;
// initialize sensors
off = new GuardedSensor(factory.findSensor("off_sensor"));
stateA = new GuardedSensor(factory.findSensor("state_a_sensor"));
stateB = new GuardedSensor(factory.findSensor("state_b_sensor"));
stateC = new GuardedSensor(factory.findSensor("state_c_sensor"));
stateD = new GuardedSensor(factory.findSensor("state_d_sensor"));
executor.scheduleAtFixedRate(new Looper(), 0L, 100L, TimeUnit.MILLISECONDS);
}
private class Looper implements Runnable {
@Override
public void run() {
if (off.read()) {
stateMachine.setState(OFF);
}
if (stateA.read()) {
stateMachine.setState(currentState(State::isA) ? OFF : A_1);
}
if (stateB.read()) {
stateMachine.setState(currentState(is(B)) ? OFF : B);
}
if (stateC.read()) {
if (currentState(is(C_1))) {
stateMachine.setState(C_2);
} else if (currentState(is(C_2)) {
stateMachine.setState(OFF);
} else {
stateMachine.setState(C_1);
}
}
if (stateD.read()) {
stateMachine.setState(currentState(is(D)) ? OFF : D);
}
}
}
private boolean currentState(Predicate pred) {
return pred.test(stateMachine.getState());
}
}
In my opinion, this version is quite clean and readable. And as an added bonus, we got the FunctionalTools class that we can reuse and expand in the future.
Enjoy.
