Refactoring Java factory method

Question

There's something very unsatisfactory about this code:

/*
Given a command string in which the first 8 characters are the command name
padded on the right with whitespace, construct the appropriate kind of 
Command object.
*/
public class CommandFactory {
     public Command getCommand(String cmd) {
         cmdName = cmd.subString(0,8).trim();

         if(cmdName.equals("START")) {
             return new StartCommand(cmd);
         }
         if(cmdName.equals("END")) {
             return new EndCommand(cmd);
         }
         // ... more commands in more if blocks here
         // else it's a bad command.
         return new InvalidCommand(cmd);
     }
}

I'm unrepentant about the multiple exit points - the structure is clear. But I'm not happy about the series of near-identical if statements. I've considered making a Map of Strings to Commands:

commandMap = new HashMap();
commandMap.put("START",StartCommand.class);
// ... etc.

... then using Reflection to make instances of the appropriate class looked up from the Map. However while conceptually elegant, this involves a fair amount of Reflection code that whoever inherits this code might not appreciate - although that cost might be offset by the benefits. All the lines hardcoding values into the commandMap smell almost as bad as the if block.

Even better would be if the factory's constructor could scan the classpath for subclasses of Command, query them for String representations, and automatically add them them to its repertoire.

So - how should I go about refactoring this?

I guess some of the frameworks out there give me this kind of thing for free. Let's assume I'm not in a position to migrate this stuff into such a framework.

Answer 1

How about the following code:

public enum CommandFactory {
    START {
        @Override
        Command create(String cmd) {
            return new StartCommand(cmd);
        }
    },
    END {
        @Override
        Command create(String cmd) {
            return new EndCommand(cmd);
        }
    };

    abstract Command create(String cmd);

    public static Command getCommand(String cmd) {
        String cmdName = cmd.substring(0, 8).trim();

        CommandFactory factory;
        try {
            factory = valueOf(cmdName);
        }
        catch (IllegalArgumentException e) {
            return new InvalidCommand(cmd);
        }
        return factory.create(cmd);
    }
}

The valueOf(String) of the enum is used to find the correct factory method. If the factory doesn't exist it will throw an IllegalArgumentException. We can use this as a signal to create the InvalidCommand object.

An extra benefit is that if you can make the method create(String cmd) public if you would also make this way of constructing a Command object compile time checked available to the rest of your code. You could then use CommandFactory.START.create(String cmd) to create a Command object.

The last benefit is that you can easily create a list of all available command in your Javadoc documentation.

Answer 2

Your map of strings to commands I think is good. You could even factor out the string command name to the constructor (i.e. shouldn't StartCommand know that its command is "START"?) If you could do this, instantiation of your command objects is much simpler:

Class c = commandMap.get(cmdName);
if (c != null)
    return c.newInstance();
else
    throw new IllegalArgumentException(cmdName + " is not as valid command");

Another option is to create an enum of all your commands with links to the classes (assume all your command objects implement CommandInterface):

public enum Command
{
    START(StartCommand.class),
    END(EndCommand.class);

    private Class<? extends CommandInterface> mappedClass;
    private Command(Class<? extends CommandInterface> c) { mappedClass = c; }
    public CommandInterface getInstance()
    {
        return mappedClass.newInstance();
    }
}

since the toString of an enum is its name, you can use EnumSet to locate the right object and get the class from within.