0

I have a factory which creates actions based on parameters. They implement my Action interface.

interface Action {
    ActionResult execute() throws Exception;
}

class CopyAction implements Action {
    ...
}

class MoveAction implements Action {
    ...
}

I have around 30 different actions. My factory looks like this:

public class ActionFactory{
    private ClassA dependency1;
    private ClassB dependency2;
    private ClassC dependency3;

    private UtilClass1 utilDependency1;
    private UtilClass2 utilDependency2;

    @Inject
    public ActionFactory(ClassA dependency1, ClassB dependency2, ClassC dependency3, UtilClass1 utilDependency1, UtilClass2 utilDependency2) {
        this.dependency1 = dependency1;
        this.dependency2 = dependency2;
        this.dependency3 = dependency3;

        this.utilDependency1 = utilDependency1;
        this.utilDependency2 = utilDependency2;
    }

    Action createAction(ActionParameters parameters, ...) {
        switch (parameters.getActionType()) {
            case COPY_ACTION:
                return new CopyAction(parameters, dependenc1, dependency2, utilDependency1);
            case MOVE_ACTION:
                return new MoveAction(parameters, dependency3, utilDependency1, utilDependency2);
            // many more cases for all the other actions
        }
    }
}

I use Dependency Injection (weld) in my project and the factory is injected in the classes which need to create actions.

So now I am mixing dependency injection and new. Furthermore my factory depends on so many different things that are only used in a couple of actions creation processes. Not all dependencies are necessary for all the actions.

I don't know if this is a good approach but some how it does not feel right. Mixing di and new seems wrong. But I don't know how to do it better. I don't even know if there is a better way of doing this or is my feeling just wrong?

Martin
  • 852
  • 7
  • 20
  • Why don't you inject dependency in actions and inject actions in your factory ? – MC Ninjava Nov 27 '19 at 08:31
  • Sorry, I forgot to mention that the constructors for the actions will take the actionsparameters. I will update my code. Furthermore if I do it, I have to inject about 30 actions into the factory. Does not feel right – Martin Nov 27 '19 at 08:45
  • Oh you can inject a list of actions created as components by type (actions need to have a common suppertype) and so you do not need to actions one by one. after that you can do createAction(ActionParameters parameters){ return this.actionLists.stream().filter(e -> e.getActionType().equals(parameters.getActionType()))} – MC Ninjava Nov 27 '19 at 13:19
  • How do I do this list injection? Are you referring to `javax.enterprise.inject.Instanc`? I am also not sure what you mean by "created as component by type" Could you give me a code example please? – Martin Nov 27 '19 at 13:36
  • with @Inject annotation you can inject a List actionLists that will bring all beans of type Action. https://stackoverflow.com/questions/4009388/inject-list-of-objects-in-cdi-weld – MC Ninjava Nov 27 '19 at 13:43
  • When I tried to `@Inject @Any List` it results in an Nullpointer Exception. – Martin Nov 29 '19 at 06:52
  • @ Inject @ Any private Instance actions; // you can use a foreach loop here to find correct action to return – MC Ninjava Nov 29 '19 at 08:43
  • That's right. But the Actions are different from the ActionParameters. Your proposal with the filter does not work, since Actions does not have a type. I could place an init method in the interface but than I have to test for the class (in the filter) and use the init method. Is this really better than the code in the question? – Martin Nov 29 '19 at 13:20
  • You can add a shared method (getActionType() )in the Action interface every action have do redine it and per example the getActionType method for CopyAction will return "COPY_ACTION" so you do not have to switch parameters and you will not have to change your factory if you add another action. it respects the SOLID design. so its better code. the code of your createAction will look like for (action in actions){ if(action.getType().equals(actionParameters.getActionType())){ return action}} – MC Ninjava Nov 29 '19 at 13:27
  • Good point. When you formulate your proposal as an answer I am happy to accept it. Thank you. – Martin Nov 29 '19 at 13:35
  • you're welcome. please vote up my answers ;) – MC Ninjava Nov 29 '19 at 14:04
  • I would rather accept an answer than up voting comments ;) Please create an answer and I will accept it (and up vote it too :) ) – Martin Nov 29 '19 at 14:18

0 Answers0