gof - Refactorización del método de fábrica de Java

object factory java

Question

Hay algo muy insatisfactorio sobre este código:

/*
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);
     }
}

No me arrepiento de los múltiples puntos de salida: la estructura es clara. Pero no estoy contento con la serie de declaraciones if casi idénticas. Consideré hacer un Mapa de cadenas para los comandos:

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

... luego usa Reflection para hacer que las instancias de la clase apropiada se vean desde el Mapa. Sin embargo, aunque conceptualmente elegante, esto implica una buena cantidad de código de reflexión que quien herede este código podría no apreciar, aunque ese costo podría ser compensado por los beneficios. Todas las líneas de los valores de hardcoding en el commandMap huelen casi tan mal como el bloque if.

Aún mejor sería si el constructor de la fábrica pudiera escanear el classpath para las subclases de Command, consultarlas para las representaciones de String y agregarlas automáticamente a su repertorio.

Entonces, ¿cómo debería proceder a refactorizar esto?

Creo que algunos de los frameworks que hay allí me dan este tipo de cosas gratis. Supongamos que no estoy en condiciones de migrar este material a un marco así.

Answer 1

¿Qué tal el siguiente código?

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

El valueOf(String) de la enumeración se usa para encontrar el método de fábrica correcto. Si la fábrica no existe lanzará una IllegalArgumentException . Podemos usar esto como una señal para crear el objeto InvalidCommand .

Un beneficio adicional es que si puede hacer que el método create(String cmd) public, también podría hacer que esta forma de construir un objeto Command compile check disponible para el resto de su código. Luego puede usar CommandFactory.START.create(String cmd ) para crear un objeto Command.

El último beneficio es que puede crear fácilmente una lista de todos los comandos disponibles en su documentación de Javadoc.

Answer 2

+1 en la sugerencia de reflexión, le dará una estructura más sensata en su clase.

De hecho, podrías hacer lo siguiente (si no lo has pensado) crear métodos que correspondan al String que estarías esperando como argumento para tu método de fábrica getCommand (), entonces todo lo que tienes que hacer es reflejar e invocar ( ) estos métodos y devuelve el objeto correcto.

Answer 3

A menos que haya una razón por la que no pueden ser, siempre trato de hacer que las implementaciones de mis comandos sean sin estado. Si ese es el caso, puede agregar un método boolean identifier (String id) a su interfaz de comando que dirá si esta instancia se puede usar para el identificador de cadena dado. Entonces su fábrica podría verse algo así (nota: no compilé ni probé esto):

public class CommandFactory {
    private static List<Command> commands = new ArrayList<Command>();       

    public static void registerCommand(Command cmd) {
        commands.add(cmd);
    }

    public Command getCommand(String cmd) {
        for(Command instance : commands) {
            if(instance.identifier(cmd)) {
                return cmd;
            }
        }
        throw new CommandNotRegisteredException(cmd);
    }
}

Answer 4

Adoptar un enfoque de Convetion vs Configuration y utilizar la reflexión para buscar objetos Command disponibles y cargarlos en su mapa sería el camino a seguir. Luego tiene la capacidad de exponer nuevos Comandos sin una recompilación de la fábrica.

Answer 5

Con la excepción del

cmd.subString(0,8).trim();

parte, esto no me parece tan malo. Podría ir con el Mapa y usar la reflexión, pero, dependiendo de la frecuencia con la que agregue / cambie los comandos, es posible que no le compre mucho.

Probablemente deberías documentar por qué solo quieres los primeros 8 caracteres, o quizás cambiar el protocolo para que sea más fácil averiguar qué parte de esa cadena es el comando (por ejemplo, pon un marcador como '':'' o '';'' después de la tecla de comando palabra).

Answer 6

Iría por el mapa y la creación a través de la reflexión. Si el escaneo de la ruta de clase es demasiado lento, siempre puede agregar una anotación personalizada a la clase, tener un procesador de anotación ejecutándose en tiempo de compilación y almacenar todos los nombres de clase en los metadatos jar.

Entonces, el único error que puedes hacer es olvidarte de la anotación.

Hice algo como esto hace un tiempo, usando maven y APT .

Answer 7

La forma en que lo hago es no tener un método Factory genérico.

Me gusta usar objetos de dominio como mis objetos de comando. Como utilizo Spring MVC, este es un gran enfoque, ya que el método DataBinder.setAllowedFields me permite una gran flexibilidad para usar un único objeto de dominio para varias formas diferentes.

Para obtener un objeto de comando, tengo un método de fábrica estático en la clase de objeto de Dominio. Por ejemplo, en la clase de miembro tendría métodos como -

public static Member getCommandObjectForRegistration();
public static Member getCommandObjectForChangePassword();

Y así.

No estoy seguro de que este sea un enfoque excelente, nunca lo vi sugerido en ninguna parte y lo inventé por mi cuenta. Me gusta la idea de mantener este tipo de cosas en un solo lugar. Si alguien ve alguna razón para oponerse, por favor hágamelo saber en los comentarios ...

Answer 8

Me gusta tu idea, pero si quieres evitar la reflexión, podrías agregar instancias al HashMap:

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

Cuando necesites un comando, solo clonalo:

command = ((Command) commandMap.get(cmdName)).clone();

Y luego, configura la cadena de comando:

command.setCommandString(cmdName);

Pero usar clone () no suena tan elegante como usar reflection :(

Answer 9

No es una respuesta directa a su pregunta, pero ¿por qué no lanza InvalidCommandException (o algo similar), en lugar de devolver un objeto de tipo InvalidCommand?

Answer 10

Otro enfoque para encontrar dinámicamente la clase a cargar sería omitir el mapa explícito, e intentar construir el nombre de clase a partir de la cadena de comandos. Un caso de título y un algoritmo de concatenación podrían cambiar "START" -> "com.mypackage.commands.StartCommand", y solo usar el reflejo para tratar de crear una instancia. Fallar de alguna manera (instancia InvalidCommand o una Excepción propia) si no puede encontrar la clase.

Luego agrega comandos simplemente agregando un objeto y empiece a usarlo.

Answer 11

Pensando en esto, puedes crear pequeñas clases de instanciación, como:

class CreateStartCommands implements CommandCreator {
    public bool is_fitting_commandstring(String identifier) {
        return identifier == "START"
    }
    public Startcommand create_instance(cmd) {
        return StartCommand(cmd);
    }
}

Por supuesto, esto agrega un montón si las clases pequeñas que no pueden hacer mucho más que decir "sí, eso comienza, dame eso" o "no, no me gusta eso", sin embargo, ahora puedes volver a trabajar la fábrica para contiene una lista de esos CommandCreators y simplemente pregunte cada uno de ellos: "¿te gusta este comando?" y devuelve el resultado de create_instance del primer CommandCreator que acepta. Por supuesto, ahora parece un poco incómodo extraer los primeros 8 caracteres fuera del CommandCreator, por lo que debería volver a trabajar para que pase toda la cadena de comandos al CommandCreator.

Creo que apliqué un "Cambiar interruptor con polimorfismo" -Refactorización aquí, en caso de que alguien se pregunte sobre eso.

Answer 12

Por lo menos, su comando debe tener un getCommandString () - donde StartCommand sobrescribe para devolver "START". Entonces puedes simplemente registrarte o descubrir las clases.

Answer 13

Sugeriría evitar la reflexión si es posible. Es algo malvado.

Puede hacer que su código sea más conciso utilizando el operador ternario:

 return 
     cmdName.equals("START") ? new StartCommand  (cmd) :
     cmdName.equals("END"  ) ? new EndCommand    (cmd) :
                               new InvalidCommand(cmd);

Podrías introducir una enumeración. Convertir cada enumeración constante en una fábrica es detallado y también tiene algún costo de memoria de tiempo de ejecución. Pero puedes buscar fácilmente una enumeración y luego usarla con == o cambiar.

 import xx.example.Command.*;

 Command command = Command.valueOf(commandStr);
 return 
     command == START ? new StartCommand  (commandLine) :
     command == END   ? new EndCommand    (commandLine) :
                        new InvalidCommand(commandLine);

Answer 14

Tener este código repetitivo de creación de objetos ocultos en la fábrica no es tan malo. Si tiene que hacerse en alguna parte, al menos está todo aquí, así que no me preocuparía demasiado.

Si realmente quiere hacer algo al respecto, tal vez busque el Mapa, pero configúrelo desde un archivo de propiedades y construya el mapa a partir de ese archivo de atributos.

Sin ir por la ruta de descubrimiento classpath (sobre la cual no sé), siempre estará modificando 2 lugares: escribiendo una clase, y luego agregando un mapeo en alguna parte (fábrica, mapa de inicio o archivo de propiedades).

Answer 15

Tu mapa de cadenas de comandos, creo que es bueno. Incluso podría factorizar el nombre del comando de cadena para el constructor (es decir, ¿no debería saber StartCommand que su comando es "START"?) Si puede hacer esto, la creación de instancias de sus objetos de comando es mucho más simple:

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

Otra opción es crear una enum de todos sus comandos con enlaces a las clases (suponga que todos sus objetos de comando implementan 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();
    }
}

dado que toString de una enumeración es su nombre, puede usar EnumSet para ubicar el objeto correcto y obtener la clase desde dentro.

Answer 16

Una opción sería que cada tipo de comando tenga su propia fábrica. Esto te da dos ventajas:

1) Su fábrica de genéricos no llamaría nueva. Entonces, cada tipo de comando podría en el futuro devolver un objeto de una clase diferente de acuerdo con los argumentos que siguen al relleno de espacio en la cadena.

2) En su esquema HashMap, podría evitar la reflexión por, para cada clase de comando, la asignación a un objeto que implementa una interfaz SpecialisedCommandFactory, en lugar de mapear a la clase misma. Este objeto en la práctica probablemente sea un singleton, pero no necesita ser especificado como tal. Su getCommand genérico luego llama al getCommand especializado.

Dicho esto, la proliferación de fábricas puede salirse de control, y el código que tienes es lo más simple que hace el trabajo. Personalmente, probablemente lo dejaría como está: puedes comparar listas de comandos en fuente y especificación sin consideraciones no locales, como lo que anteriormente se llamaba CommandFactory.registerCommand, o qué clases se descubrieron a través de la reflexión. No es confuso Es muy poco probable que sea lento por menos de mil comandos. El único problema es que no puede agregar nuevos tipos de comando sin modificar la fábrica. Pero la modificación que harías es simple y repetitiva, y si te olvidas de hacerlo obtienes un error obvio para las líneas de comando que contienen el nuevo tipo, por lo que no es oneroso.

Answer 17

Ve con tu instinto y reflexiona. Sin embargo, en esta solución, ahora se supone que su interfaz de Comando tiene acceso al método setCommandString (String s), por lo que newInstance es fácilmente utilizable. Además, commandMap es cualquier mapa con String keys (cmd) para las instancias de la clase Command a las que corresponden.

public class CommandFactory {
     public Command getCommand(String cmd) {
        if(cmd == null) {
            return new InvalidCommand(cmd);
        }

        Class commandClass = (Class) commandMap.get(cmd);

        if(commandClass == null) {
            return new InvalidCommand(cmd);
        }

        try {
            Command newCommand = (Command) commandClass.newInstance();
            newCommand.setCommandString(cmd);
            return newCommand;
        }
        catch(Exception e) {
            return new InvalidCommand(cmd);
     }
}

Answer 18

Hmm, navegando, y solo encontré esto. ¿Todavía puedo comentar?

En mi humilde opinión, no hay nada de malo con el código original de bloque if / else. Esto es simple, y la simplicidad siempre debe ser nuestra primera llamada en el diseño ( http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork )

Esto parece especialmente cierto, ya que todas las soluciones ofrecidas son mucho menos auto documentadas que el código original ... Quiero decir, ¿no deberíamos escribir nuestro código para leer en lugar de para traducir ...