HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Factory for classes unknown at compile time

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
compilefactoryunknowntimeforclasses

Problem

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 parts = new ArrayList();

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

int i;
for (i = 0; i parts = new ArrayList();
}

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

abstract class Action
{
abstract public void execute(T part);
}

class ActionFactory
{

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

for (Action action : actions)
{
if (action.getClass() == c)

Solution

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:

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

List parts = new ArrayList();


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

Some other ideas to consider:

-
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.

-
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).

-
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;
}

Code Snippets

List<Part> parts = new ArrayList<Part>();
Action rotate = new Rotate(gear);
for (final Action action: actions) {
    action.execute();
}
public class Gear {

    ...

    @Action
    public void rotate() {
        ...
    }
}
public class Action {
    private Part part;
    private String actionMethodName;
}

Context

StackExchange Code Review Q#41725, answer score: 4

Revisions (0)

No revisions yet.