Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a view which takes a DataPresenterStrategy instance to retrieve the proper text out of some data:

public interface class DataPresenterStrategy {

    String getDisplayString(Data data);

    String getUnit();
}

public class DistanceStrategy implements DataPresenterStrategy {

    @Override
    public String getDisplayString(Data data) {
        return data.getDistance();
    }

    @Override
    public String getUnit() {
         return "m";
    }
}

public class PowerStrategy implements DataPresenterStrategy {

    @Override
    public String getDisplayString(Data data) {
        return data.getPower();
    }

    @Override
    public String getUnit() {
         return "W";
    }
}

Based on some enum, I know which DataPresenterStrategy to create:

enum DataPresenterStrategyType { DISTANCE, POWER }

DataPresenterStrategy toStrategy(StrategyType type) {
    switch(type) {
        case DISTANCE:
            return new DistanceStrategy();
        case POWER:
            return new PowerStrategy();
    }
}

Obviously, this is simplified. In my actual code I have 25 of those Strategy classes. Is there a better way to do this DataPresenterStrategy instance creation?

Usage in views:

public interface DataPresenter {
    void setDataPresenterStrategy(DataPresenterStrategy strategy);
}

public class DataView implements DataPresenter {

    private DataPresenterStrategy mDataPresenterStrategy;

    @Override
    public void setDataPresenterStrategy(DataPresenterStrategy strategy) {
        mDataPresenterStrategy = strategy;
    }

}

public class MyApplication {

    private DataPresenter[] mDataPresenters;

    public void setDataPresenterStrategy(int dataPresenterIndex, DataPresenterStrategyType type) {
        switch (type)
          /* (...) */
    }

}

setDataPresenterStrategy(int, DataPresenterStrategyType) is called when a user selects an item from a spinner:

class MyOnItemSelectedListener implements OnItemSelectedListener {

    private int mPresenterIndex;

    public OnItemSelectedListener(int presenterIndex) {
        mPresenterIndex = presenterIndex;
    }

    void onItemSelected(int which) {
         setDataPresenterStrategy(mPresenterIndex, DataPresenterStrategyType.values()[which];
    } 

}
share|improve this question
    
Can you please replace the code in question with real code opposite to example code? We're not asking to show all 25 classes, but show two or three classes that are real. –  skiwi Jul 30 at 10:05
    
This is all there is to it. Only names have changed. –  Niek Haarman Jul 30 at 10:06
    
Yes, this is good code, the other option you can use DI container to create the instance, –  paritosh Jul 30 at 10:06
    
Then please change the names back, as it is right now, this question would need to be closed (unfortunately) under example code, as read in the Help center. The question looks good to me once it is fixed. Can you possibly also provide a usage example of this class? Then we could review it in a bigger context. –  skiwi Jul 30 at 10:07
    
@skiwi There you go! –  Niek Haarman Jul 30 at 10:20

2 Answers 2

up vote 5 down vote accepted
enum DataPresenterStrategyType { DISTANCE, POWER }

DataPresenterStrategy toStrategy(StrategyType type) {
    switch(type) {
        case DISTANCE:
            return new DistanceStrategy();
        case POWER:
            return new PowerStrategy();
    }
}

What I think you are looking for here is an EnumMap. If your strategies are stateless (as appears to be the case in your examples), then you simply initialize a map with all of your strategies, and use StrategyType to retrieve the one you want.

enum DataPresenterStrategyType { DISTANCE, POWER }

static EnumMap<DataPresenterStrategyType, DataPresenterStrategy> lookupStrategy = new EnumMap();
{
    lookupStrategy.put(DISTANCE, new DistanceStrategy());
    lookupStrategy.put(POWER, new PowerStrategy());
}


DataPresenterStrategy toStrategy(StrategyType type) {
    return lookupStrategy.get(type);
}

If DataPresenterStrategy cannot be shared, then you add a Factory as an extra level of indirection.

enum DataPresenterStrategyType { DISTANCE, POWER }

static EnumMap<DataPresenterStrategyType, DataPresenterStrategyFactory> lookupFactory = new EnumMap();
{
    ...
}


DataPresenterStrategy toStrategy(StrategyType type) {
    return lookupFactory.get(type).newInstance();
}
share|improve this answer

Another way to do the instance creation would be to give each type a constructor reference for the strategy it corresponds to.

enum DataPresenterStrategyType { 
    DISTANCE(DistanceStrategy::new), POWER(PowerStrategy::new);
    private final Supplier<DataPresenterStrategy> constructor;

    DataPresenterStrategyType(Supplier<DataPresenterStrategy> constructor){
        this.constructor = constructor;
    }

    DataPresenterStrategy newStrategy() {
        return constructor.get();
    }
}

The main advantages I see for this way are the compile time safety that each type has a Supplier and that you don't need to look for all the switch statements in your project to add cases if you add a new strategy type.

If not all of the constructors of your strategies take the same arguments you can replace the constructor reference with a lambda.

If you don't have access to Java 8, you can accomplish the same thing more verbosely with an anonymous class instead of the constructor reference, or you could make newStrategy() an abstract method and implement it in each type.

share|improve this answer
    
+1 for the Java 8 approach, although I can't use it. –  Niek Haarman Jul 31 at 18:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.