método de fábrica refatoração Java

Question

Há algo muito insatisfatório 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);
     }
}

Eu estou arrependido sobre os vários pontos de saída - a estrutura é clara. Mas eu não estou feliz com a série de quase idênticos se declarações. Já pensou em fazer um mapa de Cordas de comandos:

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

... em seguida, usando reflexão para fazer as instâncias da classe apropriada olhou para cima a partir do Mapa. No entanto, enquanto conceitualmente elegante, isso envolve uma quantidade razoável de código de Reflexão que quem herda este código não pôde apreciar - embora esse custo pode ser compensado pelos benefícios. Todas as linhas de codificar valores para o cheiro commandMap quase tão ruim quanto a se o bloco.

Ainda melhor seria se o construtor da fábrica poderia digitalizar o classpath para subclasses de comando, consulta-los para representações de corda, e adicioná-los automaticamente-los para o seu repertório.

Assim - como eu deveria ir sobre refatoração este

?

Eu acho que alguns dos quadros lá fora me dar esse tipo de coisa de graça. Vamos supor que eu não estou em posição de migrar esse material em quadro tal.

Answer 1

O seu mapa de cordas aos comandos eu acho que é bom. Você poderia até mesmo fator fora o nome do comando string para o construtor (ou seja, não deve StartCommand saber que o seu comando é "Iniciar"?) Se você pudesse fazer isso, instanciação de seu comando objetos é muito mais simples:

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

Outra opção é criar um enum de todos os seus comandos com links para as classes (assumir todo o seu comando objetos implementar 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();
    }
}

desde o toString de um enum é o seu nome, você pode usar EnumSet para localizar o objeto certo e obter a classe de dentro.

Answer 2

Como sobre o seguinte 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);
    }
}

O valueOf(String) do enum é usado para encontrar o método de fábrica correta. Se a fábrica não existir, ele irá lançar uma IllegalArgumentException. Podemos usar isso como um sinal para criar o objeto InvalidCommand.

Um benefício adicional é que, se você pode fazer o público método create(String cmd) se você também faria esta maneira de construir um comando tempo objeto de compilação verificado disponível para o resto do seu código. Você poderia, então, usar CommandFactory.START.create(String cmd) para criar um objeto de comando.

A última vantagem é que você pode facilmente criar uma lista de todos os comandos disponíveis na documentação Javadoc.

Answer 3

Com a excepção do

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

parte, isso não parece muito ruim para mim. Você poderia ir com o mapa e usar a reflexão, mas, dependendo de quantas vezes você adicionar / comandos de mudança, isso pode não comprar-lhe muito.

Você provavelmente deve documentar por que você quer apenas os 8 primeiros caracteres, ou talvez alterar o protocolo por isso é mais fácil de descobrir qual parte dessa cadeia é o comando (por exemplo, colocar um marcador como ':' ou ';' após a palavra-chave de comando).

Answer 4

A sua não é diretamente uma resposta à sua pergunta, mas por que não lançar uma InvalidCommandException (ou algo similar), em vez de retornar um objeto do tipo InvalidCommand?

Answer 5

A menos que haja uma razão que não pode ser Eu sempre tento fazer o meu implementações comando apátrida. Se for esse o caso, você pode adicionar um identificador de método booleano (String id) método para a sua interface de comando que iria dizer se essa instância poderia ser usado para o dado identificador de cadeia. Em seguida, sua fábrica poderia ser algo como isto (nota: eu não compilar ou testar isso):

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 6

Eu gosto da sua ideia, mas se você quer evitar reflexão você pode adicionar em vez instâncias para o HashMap:

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

Sempre que precisar de um comando, você apenas cloná-lo:

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

E depois, você definir a cadeia de comando:

command.setCommandString(cmdName);

Mas usando clone () não soa tão elegante quanto usando a reflexão: (

Answer 7

Tomar um Convetion vs abordagem Configuração e usando a reflexão para procurar Comando disponível objetos e colocá-los na mapa seria o caminho a percorrer. Você então tem a capacidade de expor novos comandos sem uma recompilação da fábrica.

Answer 8

Outra abordagem para encontrar dinamicamente a classe de carga, seria omitir o mapa explícito, e apenas tentar construir o nome da classe a partir da cadeia de comando. Um caso de título e algoritmo concatenate poderia se transformar em "Iniciar" -> "com.mypackage.commands.StartCommand", e apenas usar o reflexo para tentar instanciar-lo. Falha de alguma forma (exemplo InvalidCommand ou uma exceção de sua preferência) se você não consegue encontrar a classe.

Em seguida, você adicionar comandos apenas adicionando um objeto e começar a usá-lo.

Answer 9

Uma opção seria para cada tipo de comando para ter sua própria fábrica. Isto dá-lhe duas vantagens:

1) Sua fábrica genérico não chamaria novo. Assim, cada tipo de comando poderia, no futuro, retornar um objeto de uma classe diferente de acordo com os argumentos seguintes o preenchimento de espaço na string.

2) No seu esquema de HashMap, você poderia evitar a reflexão, por cada classe de comando, mapeamento para um objeto implementar uma interface SpecialisedCommandFactory, em vez de mapeamento para a própria classe. Este objeto na prática, seria provavelmente um singleton, mas não precisa ser especificado como tal. Seu getCommand genérico, em seguida, chama o getCommand especializada.

Dito isso, a proliferação fábrica pode sair da mão, eo código que você tem é a coisa mais simples que faz o trabalho. Pessoalmente eu provavelmente deixá-lo como ele é: você pode comparar as listas de comando na fonte e especificação sem considerações não-locais, como o que poderia ter anteriormente chamado CommandFactory.registerCommand, ou o que as classes foram descobertos através da reflexão. Não é confuso. É muito pouco provável que seja lento para menos de um milhar de comandos. O único problema é que você não pode adicionar novos tipos de comando sem modificar a fábrica. Mas a modificação que você faria é simples e repetitivo, e se você esquecer de fazê-lo você receber um erro óbvio para linhas de comando contendo o novo tipo, por isso não é oneroso.

Answer 10

Tendo este código a criação do objeto repetitivo todos escondidos na fábrica não é tão ruim. Se ele tem que ser feito em algum lugar, pelo menos, está tudo aqui, então eu não me preocuparia muito sobre isso.

Se você realmente quer fazer algo sobre isso, talvez ir para o mapa, mas configure-lo a partir de um arquivo de propriedades, e construir o mapa a partir desse ficheiro adereços.

Sem ir a rota descoberta classpath (sobre o qual eu não sei), você vai ser sempre modificando 2 lugares:. Escrever uma classe, e em seguida, adicionando um lugar de mapeamento (fábrica, mapa de inicialização, ou propriedades de arquivo)

Answer 11

Pensando nisso, Você poderia criar pequenas classes de instanciação, como:

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

Claro, isso acrescenta um grupo inteiro se pequenas classes que não pode fazer muito mais do que dizer "Sim, isso é começar, dá-me isso" ou "Não, não gosto disso", no entanto, agora você pode retrabalho a fábrica para conter uma lista das CommandCreators e apenas pedir a cada dele: "você gosta deste comando" e retornar o resultado de create_instance do primeiro CommandCreator aceitar. Claro que agora parece meio desajeitado para extrair os primeiros 8 caracteres fora do CommandCreator, por isso gostaria de retrabalho que assim que você passar toda a cadeia de comando para a CommandCreator.

Eu acho que eu apliquei alguns "switch Substituir com polimorfismo" -Refactoring aqui, no caso de alguém maravilhas sobre isso.

Answer 12

Eu iria para o mapa e criação via reflexão. Se digitalizar o caminho da classe é muito lento, você sempre pode adicionar uma anotação personalizada para a classe, tem um processador de anotação em execução em tempo de compilação e armazenar todos os nomes de classe nos metadados jar.

Então, o único erro que você pode fazer é esquecer a anotação.

Eu fiz algo parecido com isso há um tempo atrás, com o Maven e APT .

Answer 13

A forma como eu fazer isso é não ter um método de fábrica genérico.

Eu gosto de usar objetos de domínio como o meu comando objetos. Desde que eu uso Spring MVC esta é uma ótima abordagem desde o método DataBinder.setAllowedFields me permite uma grande flexibilidade de usar um único objeto de domínio para várias formas diferentes.

Para obter um objeto de comando, eu tenho um método de fábrica estático na classe de objeto Domínio. Por exemplo, na classe membro eu teria métodos como -

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

E assim por diante.

Eu não tenho certeza que esta é uma excelente abordagem, eu nunca vi isso sugeriu em qualquer lugar e tipo de só veio com isso no meu próprio b / c Eu gosto da idéia de manter as coisas como esta em um lugar. Se alguém vê qualquer razão para objeto por favor deixe-me saber nos comentários ...

Answer 14

gostaria de sugerir evitando reflexão, se possível. É um pouco mal.

Você pode fazer seu código mais conciso usando o operador ternário:

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

Você poderia introduzir uma enumeração. Fazendo cada constante enum uma fábrica é detalhado e também tem algum custo de memória de tempo de execução. Mas você pode eaily lookup um enum e usar esse com == ou switch.

 import xx.example.Command.*;

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

Answer 15

Vá com seu intestino, e refletir. No entanto, nesta solução, sua interface de comando é assumido agora ter a setCommandString (String s) método acessível, de modo que newInstance é facilmente utilizável. Além disso, commandMap é qualquer mapa com chaves de corda (CMD) para instâncias de classe de comando que correspondem a.

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 16

Hmm, navegando, e só agora me deparei com este. Ainda posso comentário?

IMHO há nada errado com o original, se / código de bloco mais. Isto é simples, e simplicidade sempre deve ser a nossa primeira chamada em design ( http://c2.com/cgi / wiki? DoTheSimplestThingThatCouldPossiblyWork )

Isto parece esp verdadeiro como todas as soluções oferecidas são muito menos auto documentar que o código original ... Eu quero dizer que não devemos escrever nosso código para a leitura em vez de tradução ...

Answer 17

No mínimo, o comando deve ter um GetCommandString () - onde substituições StartCommand retornar "Iniciar". Então você pode apenas registar-se ou descobrir as classes.

Answer 18

+1 sobre a sugestão reflexão, ele vai te dar uma estrutura mais sã em sua classe.

Na verdade, você pode fazer o seguinte (se você ainda não pensou sobre isso já) criar métodos correspondente à seqüência que você estaria esperando como um argumento para o seu método de fábrica getCommand (), então tudo que você tem a fazer é refletir e invoke () estes métodos e retornar o objeto correto.