Reputation: 60
At the moment I don't know how to avoid code smells in my piece of code. I've tried several patterns (Strategy, Visitor) and they didn't provide a clean and maintainable solution. Here is an example of my code for strategy pattern:
public interface Strategy {
<T> T foo(FirstParam firstParam, SecondParam secondParam);
}
public class StrategyOne implements Strategy {
FirstReturnType foo(FirstParam firstParam, SecondParam secondParam);
}
public class StrategyTwo implements Strategy {
SecondReturnType foo(FirstParam firstParam, SecondParam secondParam);
}
@Setter
public class Context {
private Strategy strategy;
public void execute(FirstParam firstParam, SecondParam secondParam) {
if (strategy != null) {
strategy.fo(firstParam, secondParam);
}
}
}
And there is a example of objects.
public abstract class Action {
abstract void bar();
}
public class ActionOne extends Action {
void bar() {}
}
public class ActionTwo extends Action {
void bar() {}
}
And I want to make this piece of code cleaner
public class ActionExecutor {
private Context context;
private FirstParam firstParam;
private SecondParam secondParam;
public ActionExecutor(FirstParam firstParam, SecondParam secondParam) {
this.context = new Context();
this.firstParam = firstParam;
this.secondParam = secondParam;
}
public void doSmth(Item item) {
Action action = item.getAction();
if(action instanceof ActionOne) {
context.setStrategy(new StrategyOne());
}
if(action instanceof ActionTwo) {
context.setStrategy(new StrategyTwo());
}
context.execute(firstParam, secondParam);
}
}
The idea is to perform a specific action for a specific object type. But I don't know how to avoid the usage of instanceof in this situation.
Upvotes: 3
Views: 917
Reputation: 2535
This seems like a textbook use case for the Factory Method pattern. You can use the same code you have now (or the Map example in another answer), but put it in a factory - then it's purpose-specific and decoupled from the code that uses it.
Something like this.
public class StrategyFactory {
public static Stategy getStrategy(Action action) {
if(action instanceof ActionOne) {
return new StrategyOne();
} else if(action instanceof ActionTwo) {
return new StrategyTwo();
}
}
}
And then, something like this.
Action action = item.getAction();
action.setStrategy(StrategyFactory.getStrategy(action));
There's another example here: https://dzone.com/articles/design-patterns-the-strategy-and-factory-patterns
Upvotes: 0
Reputation: 1745
Could be something like this:
public void doSmth(Item item) {
Action action = item.getAction();
Map<String,Strategy> strategies = new HashMap<>();
strategies.put(ActionOne.getClass().getSimpleName(),new StrategyOne());
strategies.put(ActionTwo.getClass().getSimpleName(),new StrategyTwo());
..
strategies.put(ActionHundred.getClass().getSimpleName(),new StrategyHundred());
if(strategies.containsKey(action.getClass().getSimpleName())) {
context.setStrategy(strategies.get(action.getClass().getSimpleName()));
}
context.execute(firstParam, secondParam); }
Upvotes: 2
Reputation: 26342
Two ways top of my head.
public abstract class Action {
public Strategy strategy;
abstract void bar();
}
public class ActionOne extends Action {
void bar() {}
// set strategy here, possibly
}
public class ActionTwo extends Action {
void bar() {}
}
public void doSmth(Item item) {
Action action = item.getAction();
action.strategy.execute(firstParam, secondParam);
}
Second way, have an enum in all your actions and force it by declaring it as a parameter in your abstract class constructor. Then just use switch in instead of instanceof
Upvotes: 2