método de fábrica de Java Refactoring

Question

Hay algo muy satisfactorio 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); 
    } 
} 

estoy arrepentido acerca de los múltiples puntos de salida - la estructura es clara. Pero no estoy contento con la serie de declaraciones if casi idénticas. He pensado en hacer un mapa de Cuerdas de Comandos:

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

... a continuación, utilizando la reflexión para que las instancias de la clase apropiada levantó la vista del 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.

Sería aún mejor 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 debo proceder para refactorizar esto?

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

Answer 1

Tu mapa de cadenas de comandos, creo que es bueno. Incluso se puede factorizar el nombre del comando de cadena al constructor (? Es decir, no debe StartCommand saber que su orden es "START") Si usted puede hacer esto, creación de instancias de los 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 un enum de todos sus comandos con enlaces a las clases (asumen 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(); 
    } 
} 

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

Answer 2

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

Answer 3

Tomar un enfoque de Convetion vs Configuration y usar 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 4

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

Answer 5

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 vaya por el Mapa, pero configúrelo desde un archivo de propiedades, y construya el mapa desde ese archivo de utilería.

Sin ir por la ruta de descubrimiento classpath (que no conozco), siempre estará modificando 2 lugares: escribiendo una clase, y luego agregando un mapeo en algún lugar (fábrica, mapa init o archivo de propiedades).

Answer 6

Con la excepción de la parte

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

, esto no se ve tan mal a mí. 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 deba documentar por qué solo quiere los primeros 8 caracteres, o tal vez cambiar el protocolo para que sea más fácil averiguar qué parte de esa cadena es el comando (por ejemplo, poner un marcador como ':' o ';' después la palabra clave de comando).

Answer 7

me gusta su idea, pero si se quiere evitar la reflexión se podría añadir en lugar casos a la HashMap:

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

Siempre que necesite un comando, simplemente clonarlo:

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

y después, se establece la cadena de comandos:

command.setCommandString(cmdName); 

Pero el uso de clone() no suena tan elegante como utilizando la reflexión :(

Answer 8

Pensando en esto, se podía crear pequeños clases de instancias, 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 se suma a un montón si diminutos clases que no se puede hacer mucho más que decir "sí , eso es comenzar, dame ese "o" no, no me gusta eso ", sin embargo, ahora puedes volver a trabajar en la fábrica para contener una lista de esos CommandCreators y solo preguntarle a cada uno:" ¿te gusta este comando? "y regresar 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é algunos "Cambiar interruptor con polimorfismo" -Refactorización aquí, en caso de que alguien se pregunte sobre eso.

Answer 9

Me gustaría ir 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 puede hacer es olvidarse de la anotación.

Hice algo así hace un tiempo, usando maven y APT.

Answer 10

A menos que haya una razón por la que no pueden ser, siempre intento 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. Y tu fábrica podría ser algo como esto (nota: yo no compilar o probar 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 11

Otro enfoque para encontrar de forma dinámica la clase a la carga, sería omitir el mapa explícita, y sólo tratar de construir el nombre de clase de la cadena de comando. 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 12

¿Y 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 utiliza para encontrar el método de fábrica correcta. Si la fábrica no existe arrojará un 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) sea público, si también haría que esta forma de construir un objeto Command compilara el tiempo 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 13

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

1) Su fábrica genérica 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 de HashMap, puede evitar la reflexión por cada clase de comando mapeando 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 tiene 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 14

La forma en que lo hago es no tener un método genérico de Fábrica.

Me gusta usar objetos de dominio como mis objetos de comando. Como utilizo Spring MVC, este es un gran enfoque ya que el DataBinder.setAllowedFields method 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í sucesivamente.

No estoy seguro de que este sea un enfoque excelente, nunca lo he visto sugerido en ninguna parte y me he dado cuenta de ello por sí solo. 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 15

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

En realidad, usted podría hacer lo siguiente (si no lo ha pensado ya) crear los métodos correspondientes al String que esperaría como argumento para su método de fábrica getCommand(), entonces todo lo que tiene que hacer es reflejar e invocar() estos métodos y devolver el objeto correcto.

Answer 16

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

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

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

Se podría 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 17

Vaya con su intestino y reflexione. 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 acabo de enterarme de esto. ¿Todavía puedo comentar?

EN MIEMO hay nada mal con el código de bloque if/else original. 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 traducción ...