Refactor big switch case combined with if clauses

Question

So I'm having something like this:

switch (data){
    case "one" :
        if (version1) 
            createDataOneV1();
         else 
             createDataOnetV2();
         break;
    case "two":
        if(version1)
            createDataTwoV1();
        else 
            createDataTwoV2();
        break;
    default:
        createDefaultData();
}

The are a lot of (switch) cases. Any recommendations for a refactoring?

You could add a `createData` method taking `(String data, boolean versionFlag)` - it would then branch depending on both arguments, and invoke the `createData`methods. Still very ugly but your whole `switch` goes away. — Mena, Oct 24 '19 at 14:37

ETO · Answer 1 · 2019-10-24T16:31:41.173

Do not use Map. It will not give you the same compile time safety as your switch does.

If you still want to get rid of it, then I would suggest using enum:

public enum DataCreationStrategy {

    ONE("one", DataCreator::createDataOneV1, DataCreator::createDataOneV2),
    TWO("two", DataCreator::createDataTwoV1, DataCreator::createDataTwoV2)
    // ... other cases
    ;

    private final String key;
    private final Function<DataCreator, String> creator;
    private final Function<DataCreator, String> defaultCreator;

    DataCreationStrategy(String key, Function<DataCreator, String> creator, Function<DataCreator, String> defaultCreator) {
        this.key = key;
        this.creator = creator;
        this.defaultCreator = defaultCreator;
    }

    public static Function<DataCreator, String> of(String key, boolean flag) {
        for (DataCreationStrategy strategy: values()){
            if(strategy.key.equals(key)){
                return flag ? strategy.creator : strategy.defaultCreator;
            }
        }
        return DataCreator::createDefaultData;
    }

}

Then use it like this:

String createdData = DataCreationStrategy.of(key, versionFlag).apply(creator);

(You can replace String to the actual data type you need to generate)

The of method could be implemented in Stream API fashion also. But plain old for-loop is much cleaner in this particular case.

public static Function<DataCreator, String> of(String key, boolean flag) {
   return Arrays.stream(values())
                .filter(s -> s.key.equals(key))
                .findAny()
                .map(s -> flag ? flag ? s.creator : s.defaultCreator )
                .orElse(DataCreator::createDefaultData);
}

But what if only some of the data has V3? – UnguruBulan Oct 25 '19 at 07:55 — UnguruBulan, Oct 25 '19 at 07:55

score 1 · Answer 2 · answered Oct 24 '19 at 14:35

1

Create Map with Strings as keys and functions as a values.

answered Oct 24 '19 at 14:35

Lareth51

31
3

1

might just be better as `Map>` depending on the `createData***` method. – Naman Oct 24 '19 at 14:37
4

@Naman or `Map, Consumer/Supplier/Function>` or `Map`… – Holger Oct 24 '19 at 14:47

score 1 · Answer 3 · answered Oct 24 '19 at 14:57

You could create a Key for a HashMap with some Supplier (for example):

@RequiredArgsConstructor
class SpecialKey  {
     private final String data;
     private final boolean second;
     // hashCode/equals
}

Let's assume your createDataOneV1 and createDataOneV2 return a boolean:

Map<SpecialKey, Supplier<Boolean>> map = new HashMap<>();
map.put(new SpecialKey("one", true), this::createDataOneV1);
map.put(new SpecialKey("one", false), this::createDataOneV2);

And then simply:

String data = ... // get the "data" somehow
boolean version = ...

boolean res = map.getOrDefault(new SpecialKey(data, version), this::createDefaultData).get();

score 1 · Answer 4 · answered Oct 24 '19 at 14:59

First decide whether you really need to refactor that code. Is it ugly? Sure. But it's simple and straightforward. You could clean it up using an additional layer of abstraction, but that typically comes at a cost of making the code less obvious, and often less performant.

Anyway, if you want to refactor that code, you could use the type system. Define a DataCreator interface which can generate data for V1 and V2. You can chose the right DataCreator using a function like this:

DataCreator getDataCreatorForData(String data){
    switch(data) {
       case "one": return new DataCreatorOne();
       case "two": return new DataCreatorTwo();
       default: return new DefaultDataCreator()
    }
}

And once you have the DataCreator, you check the version:

DataCreator creator = getDataCreatorForData(data);
if (version1){
    creator.createV1Data();
}else{
    creator.createV2Data();
}

This structure is flatter than the one in the question, less nested.

Refactor big switch case combined with if clauses

4 Answers4