Let’s see a legacy code example (note: even it is just an example, but it comes from a real project.
package de.jingge.refactoring;
public class SystemManager {
public static final int LOGGEDIN = 0;
public static final int LOGGEDOUT = 1;
public static final int IDLE = 2;
int state;
public void login() {
// call service#login()
updateState(LOGGEDIN);
}
public void logout() {
// call service#logout()
updateState(LOGGEDOUT);
}
public void idle() {
// call some other services
updateState(IDLE);
}
public void updateState(int state) {
if (state == LOGGEDIN) {
// do something after logging in is successful,
// for example: show welcome dialog, open the last edit document, etc.
} else if (state == LOGGEDOUT) {
// do something after logging out is successful,
// for example: free used resource, dispose GUI components, etc.
} else if (state == IDLE) {
// do something after the user is idle,
// for example: save the application state temporarily, lock the application, etc.
} else {
throw new IllegalArgumentException("unknown state");
}
this.state = state;
}
}
Here we get a SystemManager, which is responsible for managing the system state, i.e. logged in, logged out, idle. As you can see, this is legacy class, which use the “int” for presenting the system state and has an “If-else hell” in the method updateState(int state). With growing number of the states we will get more pains.
How can we refactoring this code and make it more object oriented?
First, we should create a unit test. Since the SystemManager class was created, we are not doing here 100% TDD. But we can still hold the TDD soul.
The first unit test looks like this:
package de.jingge.refactoring;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import static org.junit.Assert.*;
public class SystemManagerTest {
private static SystemManager manager;
@BeforeClass
public static void setUpClass() throws Exception {
manager = new SystemManager();
// add some service mock objects
}
@AfterClass
public static void tearDownClass() throws Exception {
}
@Test
public void login() {
manager.login();
assertEquals(manager.state, SystemManager.LOGGEDIN);
}
@Test
public void logout() {
manager.logout();
assertEquals(manager.state, SystemManager.LOGGEDOUT);
}
@Test
public void idle() {
manager.idle();
assertEquals(manager.state, SystemManager.IDLE);
}
}
Run the test, it passt:
Now let’s begin refactoring:
Replace the legacy integer constant with java Enum
This Step is relatively easy. Just create a new enum class:
package de.jingge.refactoring;
public enum SystemState {
LOGGEDIN,
LOGGEDOUT,
IDLE;
}
It is simple to replace all code calling the integer constant with calling the enum instance:
1. Add import static de.jingge.refactoring.SystemState.*;
2. Remove all integer constants.
3. Change the type of the state to the enum class SystemState.
After the refactoring, the code should look like this:
package de.jingge.refactoring;
import static de.jingge.refactoring.SystemState.*;
public class SystemManager {
SystemState state;
public void login() {
// call service#login()
updateState(LOGGEDIN);
}
public void logout() {
// call service#logout()
updateState(LOGGEDOUT);
}
public void idle() {
// call some other services
updateState(IDLE);
}
public void updateState(SystemState state) {
if (state == LOGGEDIN) {
// do something after logging in is successful,
// for example: show welcome dialog, open the last edit document, etc.
} else if (state == LOGGEDOUT) {
// do something after logging out is successful,
// for example: free used resource, dispose GUI components, etc.
} else if (state == IDLE) {
// do something after the user is idle,
// for example: save the application state temporarily, lock the application, etc.
} else {
throw new IllegalArgumentException("unknown state");
}
this.state = state;
}
}
And in the test class:
1. Add import static de.jingge.refactoring.SystemState.*;
2. Remove the SystemManager reference before all constants.
package de.jingge.refactoring;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import static org.junit.Assert.*;
import static de.jingge.refactoring.SystemState.*;
/**
*
* @author JGe
*/
public class SystemManagerTest {
private static SystemManager manager;
@BeforeClass
public static void setUpClass() throws Exception {
manager = new SystemManager();
// add some service mock objects
}
@AfterClass
public static void tearDownClass() throws Exception {
}
@Test
public void login() {
manager.login();
assertEquals(manager.state, LOGGEDIN);
}
@Test
public void logout() {
manager.logout();
assertEquals(manager.state, LOGGEDOUT);
}
@Test
public void idle() {
manager.idle();
assertEquals(manager.state, IDLE);
}
}
Run the test again. It should be passed.
Now we should go to handle the “if-else hell”.
Get ride of the if-else hell
Take a look at the the updateState() method. The reason, why if-else is used, is that the SystemState is just an enum and does not has any activities. After caught this point, it should be clear how to refactoring this bad code: change the input the parameter of the updateState() method into a activity class and move the logic code into that class. By calling the appropriate method of such class in the updateState() method, the if-else will disappear.
We call these activity classes as System State Performer, the base class, which should be abstract, should look like this:
package de.jingge.refactoring;
import java.awt.Image;
public abstract class SystemStatePerformer {
private SystemState state;
private Image image;
public SystemStatePerformer(SystemState state, Image image) {
this.state = state;
this.image = image;
}
public SystemState getState() {
return state;
}
public Image getImage() {
return image;
}
public abstract void perform();
}
Like the code says, each performer has a SystemState and can contain any other object references. The Image showed here is such an example, which means each state has its own image shown to the user. The perform() method will do the real business logic for it’s state.
The next step is building concrete Performer for each state. The first idea came to my mind is building sub class for each state. You can do it, if you like this style. But, in my opinion, this way will make a lot of classes and we will easily lose the focus. I decide to use a Factory and create anonymous class for each state. Doing it this way can hold all classes for all state in a one location. Since adding support for new state will force changing at lease one class, why not choose the factory class?
The performer factory class should look like this:
package de.jingge.refactoring;
import static de.jingge.refactoring.SystemState.*;
import java.awt.Image;
import java.awt.image.BufferedImage;
public class SystemStatePerformerFactory {
private static SystemStatePerformerFactory INSTANCE = new SystemStatePerformerFactory();
private SystemStatePerformerFactory() {
}
public static SystemStatePerformer getSystemStatePerformer(SystemState state) {
switch (state) {
case LOGGEDIN:
return createLoggedInPerformer();
case IDLE:
return createIdlePerformer();
case LOGGEDOUT:
return createLoggedOutPerformer();
default:
throw new IllegalAccessError("Unkonw status");
}
}
private static SystemStatePerformer createLoggedInPerformer() {
return new SystemStatePerformer(LOGGEDIN, getImage("loggedin.gif")) {
@Override
public void perform() {
// do something after logging in is successful,
// for example: show welcome dialog, open the last edit document, etc.
}
};
}
private static SystemStatePerformer createLoggedOutPerformer() {
return new SystemStatePerformer(LOGGEDOUT, getImage("loggedout.gif")) {
@Override
public void perform() {
// do something after logging out is successful,
// for example: free used resource, dispose GUI components, etc. }
}
};
}
private static SystemStatePerformer createIdlePerformer() {
return new SystemStatePerformer(IDLE, getImage("idle.gif")) {
@Override
public void perform() {
// do something after the user is idle,
// for example: save the application state temporarily, lock the application, etc.
}
};
}
private static Image getImage(String string) {
return new BufferedImage(10, 10, BufferedImage.TYPE_4BYTE_ABGR);
}
}
In this factory, for each state, there is a performer create method, which creates the anonymous class. The different business logic code for different state are moved into the appropriate perform() method. There is only one public method getSystemStatePerformer(), which can be called by our SystemManager.
Note, since I want to concentrate on the if-else problem, there are some bad design in this class. First, the getImage() method is just used as example. In a real project, it should be more type save. Second, the static method will make the test painful. In a real project, you should avoid using it. Try DI framework, for example google guice for solving this problem.
Ok, now we have refactoring the conditional code out from the SystemManager into the SystemStatePerformer. The next step is refactoring the SystemManager, replace the SystemState with the performer:
1. Change the variable SystemState state to SystemStatePerformer statePerformer. Note: use the IDE refatoring, because the state is used in the test.
2. Call the SystemStatePerformerFactory in the updateState() method
3. Calling getState() after manager.statePerformer in the test, because we test the state not the statePerformer.
After the refactoring, the SystemManager looks like this:
package de.jingge.refactoring;
import static de.jingge.refactoring.SystemState.*;
public class SystemManager {
SystemStatePerformer statePerformer;
public void login() {
// call service#login()
updateState(LOGGEDIN);
}
public void logout() {
// call service#logout()
updateState(LOGGEDOUT);
}
public void idle() {
// call some other services
updateState(IDLE);
}
public void updateState(SystemState state) {
this.statePerformer = SystemStatePerformerFactory.getInstance()
getSystemStatePerformer(state);
statePerformer.perform();
}
}
Like the code tells us, the if-else is disappeared.
The test class looks like this:
package de.jingge.refactoring;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import static org.junit.Assert.*;
import static de.jingge.refactoring.SystemState.*;
public class SystemManagerTest {
private static SystemManager manager;
@BeforeClass
public static void setUpClass() throws Exception {
manager = new SystemManager();
// add some service mock objects
}
@AfterClass
public static void tearDownClass() throws Exception {
}
@Test
public void login() {
manager.login();
assertEquals(manager.statePerformer.getState(), LOGGEDIN);
}
@Test
public void logout() {
manager.logout();
assertEquals(manager.statePerformer.getState(), LOGGEDOUT);
}
@Test
public void idle() {
manager.idle();
assertEquals(manager.statePerformer.getState(), IDLE);
}
}
Until now, the refactoring is almost done, the code looks more OO. But there is still one little problem: the if-else in the SystemManager disappeared, but we get a switch in the factory. It is, actually, nothing else as the if-else. It is not a pure solution.
Ok, let’s move further. For getting ride of this switch, we can instantiate all anonymous classes before the getSystemStatePerformer() is called and return the correct instance when the method is called. A Map is for this case a simple choice for holding all instances of the SystemStatePerformer.
After refactoring the SystemManager should look like this:
package de.jingge.refactoring;
import static de.jingge.refactoring.SystemState.*;
import java.awt.Image;
import java.awt.image.BufferedImage;
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
public class SystemStatePerformerFactory {
private static SystemStatePerformerFactory INSTANCE = new SystemStatePerformerFactory();
private Map<SystemState, SystemStatePerformer> performers;
private SystemStatePerformerFactory() {
}
public static SystemStatePerformerFactory getInstance() {
return INSTANCE;
}
private synchronized Map<SystemState, SystemStatePerformer> getPerformers()
throws Exception {
if (performers == null) {
performers = new HashMap<SystemState, SystemStatePerformer>();
// call all @FactoryMethod using reflection
for (Method m : getClass().getDeclaredMethods()) {
if (m.getAnnotation(FactoryMethod.class) != null) {
SystemStatePerformer p = (SystemStatePerformer) m.invoke(
this, new Object[]{});
performers.put(p.getState(), p);
}
}
// make it readonly
performers = Collections.unmodifiableMap(performers);
}
return performers;
}
public SystemStatePerformer getSystemStatePerformer(SystemState state) throws Exception{
return getPerformers().get(state);
}
@FactoryMethod
private SystemStatePerformer createLoggedInPerformer() {
return new SystemStatePerformer(LOGGEDIN, getImage("loggedin.gif")) {
@Override
public void perform() {
// do something after logging in is successful,
// for example: show welcome dialog, open the last edit document, etc.
}
};
}
@FactoryMethod
private SystemStatePerformer createLoggedOutPerformer() {
return new SystemStatePerformer(LOGGEDOUT, getImage("loggedout.gif")) {
@Override
public void perform() {
// do something after logging out is successful,
// for example: free used resource, dispose GUI components, etc. }
}
};
}
@FactoryMethod
private SystemStatePerformer createIdlePerformer() {
return new SystemStatePerformer(IDLE, getImage("idle.gif")) {
@Override
public void perform() {
// do something after the user is idle,
// for example: save the application state temporarily, lock the application, etc.
}
};
}
private Image getImage(String string) {
return new BufferedImage(10, 10, BufferedImage.TYPE_4BYTE_ABGR);
}
}
The getPerformers() method will build all performer instances lazily and will be called when the getSystemStatePerformer() is called at the first time. I create and use the @FactoryMethod annotation, because I want to avoid changing the getSystemStatePerformer() method, each time when new create***Performer() method is added.
The annotation class looks like this:
package de.jingge.refactoring;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD})
public @interface FactoryMethod {
}
Well, all are done! There is no if-else, no switch. For further state, just add a new create***Performer() method in the factory, that’s all!
Friend, who read the “Refactoring to patterns”, may say that what we did here is same as what the author described in his book, in the last segment of the chapter 7: Replace Conditional Dispatcher with Command. Well, at first glance, they are, but after read my whole code and think about it, the answer is No.
The Book Refactoring to Patterns is great. I really like it. I have read it many times. All what I did here is, actually, based on the solution written in that book. But there are still some differents:
1. Factory + anonymous class instead of the concrete sub classes.
Doing it like this can hold all performer in one class and all needed object references are holt by the factory not the concrete sub classes. This can reduce the reference complex.
2. The performer is not just a command. It has a state and could have many more logic than a command.
Further reading:
Design patterns Elements of Reusable Object-Oriented
Refactoring to Patterns
Test Driven Practical TDD and Acceptance TDD for Java Developers
No comments:
Post a Comment