Factory for classes unknown at compile time

Question

I have a class called Machine. It contains a list of parts and a list of actions. The list of parts will contain instances of the Part class subclasses and the list of actions will contain instances of my Action class subclasses. Each Part subclass will have its corresponding Action subclass. None of the subclasses are available at compile time. When a Machine instance starts all its parts should perform their action.

An action will perform the same manipulation for each Part subclass so I am trying to create a factory class that will create a single instance for each Action subclass. My first thought was to use an interface for Action, but then I would have to create an Action instance for each Part instance I create, or make each class that implemented Action responsible for making itself a singleton. I am not sure if a factory is the best solution and if I am implementing the pattern correctly.

Neither Action nor Part subclasses are available to me so I am using generics. I am new to Java so I am not sure if I am using them correctly - Eclipse gives me several Raw type warnings (I indicated where). Here is my code:

import java.util.ArrayList;

public class GenericsTest
{
    public static void main(String[] args) throws InstantiationException,
            IllegalAccessException
    {

        ArrayList<Part> parts = new ArrayList<Part>();

        Rotate rotate = ActionFactory.getAction(Rotate.class);
        Move move = ActionFactory.getAction(Move.class);

        int i;
        for (i = 0; i < 1000; i++)
        {
            Gear gear = new Gear();
            gear.value1 = i;
            gear.actions.add(rotate);
            parts.add(gear);

            Lever lever = new Lever();
            lever.value2 = i + 2000;
            lever.actions.add(move);
            parts.add(lever);
        }

        Machine machine = new Machine();
        machine.parts.addAll(parts);
        machine.start();
    }

}

class Machine
{
    public void start()
    {
        for (Part part : parts)
        {
            // Raw type warning
            for (Action action : part.actions)
            {
                // Raw type warning
                action.execute(part);
            }
        }
    }

    public ArrayList<Part> parts = new ArrayList<Part>();
}

abstract class Part
{
    // Raw type warning
    public ArrayList<Action> actions = new ArrayList<Action>();
}

abstract class Action<T extends Part>
{
    abstract public void execute(T part);
}

class ActionFactory<T extends Action>
{

    public static <T> T getAction(Class<T> c) throws InstantiationException,
            IllegalAccessException
    {
        T returnAction = null;

        for (Action action : actions)
        {
            if (action.getClass() == c)
            {
                // Unchecked cast warning
                returnAction = (T) action;
            }
        }

        if (returnAction == null)
        {
            returnAction = c.newInstance();
            actions.add((Action) returnAction);
        }

        return returnAction;
    }

    private static ArrayList<Action> actions = new ArrayList<Action>();
}

/*
 * Subject1, Subject2, Action1 and Action2, somewhere else:
 */

class Gear extends Part
{
    public int value1;
}

class Lever extends Part
{
    public int value2;
}

class Rotate extends Action<Gear>
{

    @Override
    public void execute(Gear part)
    {
        System.out.println("rotate action executed, value 1: " + part.value1);
    }
}

class Move extends Action<Lever>
{

    @Override
    public void execute(Lever part)
    {
        System.out.println("move action executed, value 2: " + part.value2);
    }
}

Answer 1

I don't understand exactly the purpose of the code but having two similar object inheritance tree (Actions and Parts) smells a little bit. It also seems a sample code and I guess it violates the single responsibility principle (the behaviour of a part is separated to two classes) and it might also violates the Liskov substitution principle too (if you have an Action you can't use it with any kind of Part). I'd consider moving the action code to the Part class or creating generic Action classes which ignore unhandled Parts (by class type). Anyway, it's hard to say something more useful without the purpose of the code. See also: SOLID, Parallel Inheritance Hierarchies.

Other notes:

1. ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

   List<Part> parts = new ArrayList<Part>();
   

2. Public fields usually leads to harder maintenance. See: What's the deal with Java's public fields?

Some other ideas to consider:

1. Create a separate Action instance for every part and action. Pass the Part instance to the constructor of your Action. For example:

   Action rotate = new Rotate(gear);
   

   Store the actions in a list and then you can call execute on the rotate instance:

   for (final Action action: actions) {
       action.execute();
   }
   

   It's type-safe, clients can't create an Action with a wrong Part type.

2. I'd move the actions list from the Part object to somewhere else. Why should a part know what actions does a machine do with it? It seems to me that this responsibility should be stored somewhere else. The Macine is the closest object now (but it might deserve separate class).

3. If the Action classes do not have any state consider moving their code inside the Part object. (The basic idea of OOP is that the data and the logic which operates with the data should be together in a class.) You can mark the action method with an @Action annotation and parse it runtime.

   public class Gear {
   
       ...
   
       @Action
       public void rotate() {
           ...
       }
   }
   

   I guess you still need a class which stores the actions which a machine should do:

   public class Action {
       private Part part;
       private String actionMethodName;
   }
   

Answer 2

This is a complicated setup you have, and this problem has been successfully solved in a few ways.... but, all of the mechanisms have two things in common....

1. Interfaces.
2. Factories

The three technical problems I see with your code are:

• false generics: You have the factory class ActionFactory<T extends Action> which implies that the factory has something to do with actions, and with a generic type called 'T'.

  This is simply not true.... The generic type on your class is completely ignored. You do not even need to create an instance of the class. The two static methods on the class have their own T generic type, and the T on those methods have absoutely nothing to do with the T on the class.

• even though I think the Part should be an interface, which would 'fix' this, there is no way you should have (in Part_ : public ArrayList<Action> actions = new ArrayList<Action>();. This should be a hidden (and final) field that is accessed with getters and setters:

  public void addAction(Action<? extends T> action);
  ....
  public List<Action<...>> getActions();
  

• The Part and Action<T extends Part> should both be interfaces.

This is a challenging problem to face. There are going to be places where the generics are wrong, right now they are off in ways that make the code very hard to understand.

Answer 3

I think that this ActionFactory might work for you.

• The ActionFactory has only one method, and it is static, so make the constructor private to prevent the factory from being instantiated. The ActionFactory itself needs no type parameters.
• The type parameter for getAction() should have a constraint requiring T to be a subclass of Action. Then actions.add(…) would not need to cast.
• Instead of keeping an ArrayList of Actions, keep a Map instead for a simple lookup.
• There would still be an unchecked cast warning, which I believe is unavoidable. Just suppress it with an annotation.

class ActionFactory
{
    private static final Map<Class, Action> singletons = new HashMap<Class, Action>();

    private ActionFactory() {}

    public static <T extends Action> T getAction(Class<T> c)
            throws InstantiationException, IllegalAccessException
    {
        @SuppressWarnings("unchecked")
        T action = (T)singletons.get(c);

        if (action == null)
        {
            action = c.newInstance();
            singletons.put(c, action);
        }

        return action;
    }
}