Metodo di refactoring della factory Java
https://stackoverflow.com/questions/115269
italiano
english
français
española
中国
日本の
العربية
Deutsch
한국어
Português
RussianDomanda
C'è qualcosa di molto insoddisfacente in questo codice:
/*
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);
}
}
Non sono pentito per i punti di uscita multipli - la struttura è chiara. Ma non sono contento della serie di dichiarazioni if ??quasi identiche. Ho preso in considerazione l'idea di creare una mappa di stringhe per i comandi:
commandMap = new HashMap();
commandMap.put("START",StartCommand.class);
// ... etc.
... quindi usando Reflection per fare in modo che le istanze della classe appropriata guardassero dalla Mappa. Tuttavia, sebbene concettualmente elegante, ciò comporta una discreta quantità di codice Reflection che chiunque erediti questo codice potrebbe non apprezzare, anche se tale costo potrebbe essere compensato dai benefici. Tutte le linee che codificano i valori nella commandMap hanno un odore quasi uguale al blocco if.
Ancora meglio sarebbe se il costruttore della fabbrica potesse scansionare il percorso di classe alla ricerca di sottoclassi di Command, interrogarle per le rappresentazioni di String e aggiungerle automaticamente al suo repertorio.
Quindi - come dovrei procedere con il refactoring?
Suppongo che alcuni dei framework là fuori mi offrano questo genere di cose gratuitamente. Supponiamo che non sia in grado di migrare questa roba in un tale framework.
Soluzione
La tua mappa di stringhe per i comandi penso sia buona. Potresti anche fattorizzare il nome del comando stringa al costruttore (cioè StartCommand non dovrebbe sapere che il suo comando è " START " ;?) Se potessi farlo, l'istanza dei tuoi oggetti comando è molto più semplice:
Class c = commandMap.get(cmdName);
if (c != null)
return c.newInstance();
else
throw new IllegalArgumentException(cmdName + " is not as valid command");
Un'altra opzione è quella di creare un enum di tutti i tuoi comandi con collegamenti alle classi (supponiamo che tutti i tuoi oggetti di comando implementino 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();
}
}
poiché toString di un enum è il suo nome, puoi usare EnumSet per localizzare l'oggetto giusto e ottenere la classe dall'interno.
Altri suggerimenti
Che ne dici del seguente codice:
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);
}
}
Il valueOf (String) dell'enum viene utilizzato per trovare il metodo factory corretto. Se la fabbrica non esiste, verrà generato un IllegalArgumentException . Possiamo usarlo come segnale per creare l'oggetto InvalidCommand .
Un ulteriore vantaggio è che se è possibile rendere pubblico il metodo create (String cmd) se si desidera rendere questo modo di costruire un tempo di compilazione dell'oggetto Command controllato disponibile per il resto del codice. È quindi possibile utilizzare CommandFactory.START.create (String cmd ) per creare un oggetto Command.
L'ultimo vantaggio è che puoi facilmente creare un elenco di tutti i comandi disponibili nella tua documentazione Javadoc.
Ad eccezione di
cmd.subString(0,8).trim();
parte, questo non sembra troppo male per me. Potresti andare con la Mappa e usare la riflessione, ma, a seconda della frequenza con cui aggiungi / cambi comandi, questo potrebbe non farti guadagnare molto.
Probabilmente dovresti documentare perché vuoi solo i primi 8 caratteri, o forse cambiare il protocollo in modo che sia più facile capire quale parte di quella stringa è il comando (es. metti un marcatore come ':' o ';' dopo il parola chiave di comando).
Non è direttamente una risposta alla tua domanda, ma perché non lanciare un InvalidCommandException (o qualcosa di simile), piuttosto che restituire un oggetto di tipo InvalidCommand?
A meno che non ci sia una ragione per cui non possono essere, cerco sempre di rendere apolidi le implementazioni dei miei comandi. In tal caso, puoi aggiungere un metodo identificatore booleano (ID stringa) all'interfaccia di comando che indichi se questa istanza può essere utilizzata per l'identificatore di stringa specificato. Quindi la tua fabbrica potrebbe assomigliare a questa (nota: non l'ho compilata o testata):
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);
}
}
Mi piace la tua idea, ma se vuoi evitare la riflessione potresti invece aggiungere istanze alla HashMap:
commandMap = new HashMap();
commandMap.put("START",new StartCommand());
Ogni volta che hai bisogno di un comando, devi solo clonarlo:
command = ((Command) commandMap.get(cmdName)).clone();
E successivamente, imposta la stringa di comando:
command.setCommandString(cmdName);
Ma usare clone () non sembra elegante come usare reflection :(
Prendere un approccio Convetion vs Configuration e usare la riflessione per cercare oggetti Command disponibili e caricarli sulla tua mappa sarebbe la strada da percorrere. Hai quindi la possibilità di esporre nuovi comandi senza ricompilare la fabbrica.
Un altro approccio per trovare dinamicamente la classe da caricare, sarebbe quello di omettere la mappa esplicita e provare a costruire il nome della classe dalla stringa di comando. Un caso di titolo e un algoritmo concatenato potrebbero trasformare "INIZIO" - > " com.mypackage.commands.StartCommand " ;, e usa semplicemente la riflessione per provare ad istanziarlo. Fallisci in qualche modo (istanza InvalidCommand o un'eccezione tua) se non riesci a trovare la classe.
Quindi aggiungi i comandi semplicemente aggiungendo un oggetto e inizi a usarlo.
Un'opzione sarebbe per ogni tipo di comando avere la propria fabbrica. Questo ti offre due vantaggi:
1) La tua fabbrica generica non chiamerebbe nuovo. Pertanto, in futuro, ogni tipo di comando potrebbe restituire un oggetto di una classe diversa in base agli argomenti che seguono il riempimento dello spazio nella stringa.
2) Nel tuo schema HashMap, potresti evitare la riflessione, per ogni classe di comando, mappando su un oggetto che implementa un'interfaccia SpecialisedCommandFactory, invece di mappare sulla classe stessa. Questo oggetto in pratica sarebbe probabilmente un singleton, ma non è necessario specificarlo come tale. Il tuo getCommand generico chiama quindi il getCommand specializzato.
Detto questo, la proliferazione in fabbrica può sfuggire di mano e il codice che hai è la cosa più semplice che fa il lavoro. Personalmente probabilmente lo lascerei così com'è: puoi confrontare gli elenchi di comandi nel sorgente e nelle specifiche senza considerazioni non locali come quello che avrebbe potuto precedentemente chiamare CommandFactory.registerCommand o quali classi sono state scoperte tramite la riflessione. Non è confuso. È molto improbabile che sia lento per meno di mille comandi. L'unico problema è che non è possibile aggiungere nuovi tipi di comando senza modificare la factory. Ma la modifica che faresti è semplice e ripetitiva, e se ti dimentichi di farlo, ricevi un errore evidente per le righe di comando che contengono il nuovo tipo, quindi non è oneroso.
Avere questo codice ripetitivo per la creazione di oggetti tutti nascosti in fabbrica non è poi così male. Se deve essere fatto da qualche parte, almeno è tutto qui, quindi non mi preoccuperei troppo.
Se veramente vuoi fare qualcosa al riguardo, magari scegli la Mappa, ma configurala da un file delle proprietà e costruisci la mappa da quel file props.
Senza seguire il percorso di rilevamento del percorso di classe (di cui non conosco), modificherai sempre 2 posizioni: scrivere una classe e quindi aggiungere una mappatura da qualche parte (factory, map init o file delle proprietà).
Pensando a questo, potresti creare piccole classi di istanza, come:
class CreateStartCommands implements CommandCreator {
public bool is_fitting_commandstring(String identifier) {
return identifier == "START"
}
public Startcommand create_instance(cmd) {
return StartCommand(cmd);
}
}
Ovviamente, questo aggiunge un sacco di se piccole classi che non possono fare molto di più che dire "sì, questo è il punto di partenza, dammi che" o " no, non mi piace " tuttavia, ora puoi rielaborare la fabbrica per contenere un elenco di quei CommandCreator e chiedere semplicemente a ciascuno di essi: " ti piace questo comando? " e restituisce il risultato di create_instance del primo CommandCreator che accetta. Ovviamente ora sembra un po 'complicato estrarre i primi 8 caratteri al di fuori del CommandCreator, quindi vorrei rielaborarlo in modo da passare l'intera stringa di comando al CommandCreator.
Penso di aver applicato alcuni "Sostituisci interruttore con polimorfismo", Rifattorizzando qui, nel caso qualcuno si chieda.
Vorrei cercare la mappa e la creazione tramite la riflessione. Se la scansione del percorso della classe è troppo lenta, puoi sempre aggiungere un'annotazione personalizzata alla classe, avere un processore di annotazioni in esecuzione al momento della compilazione e archiviare tutti i nomi delle classi nei metadati jar.
Quindi, l'unico errore che puoi fare è dimenticare l'annotazione.
Qualche tempo fa ho fatto qualcosa del genere, usando maven e APT .
Il modo in cui lo faccio è di non avere un metodo Factory generico.
Mi piace usare Oggetti dominio come oggetti comando. Da quando utilizzo Spring MVC questo è un ottimo approccio poiché metodo DataBinder.setAllowedFields mi consente una grande flessibilità nell'uso di un singolo oggetto di dominio per diverse forme.
Per ottenere un oggetto comando, ho un metodo factory statico sulla classe di oggetti Domain. Ad esempio, nella classe membro avrei metodi come -
public static Member getCommandObjectForRegistration();
public static Member getCommandObjectForChangePassword();
E così via.
Non sono sicuro che questo sia un ottimo approccio, non l'ho mai visto suggerito da nessuna parte e mi è venuto in mente di farlo sul mio b / c, mi piace l'idea di tenere le cose come questa in un posto. Se qualcuno vede qualche motivo di opposizione, per favore fatemi sapere nei commenti ...
Suggerirei di evitare la riflessione, se possibile. È in qualche modo malvagio.
Puoi rendere il tuo codice più conciso usando l'operatore ternario:
return
cmdName.equals("START") ? new StartCommand (cmd) :
cmdName.equals("END" ) ? new EndCommand (cmd) :
new InvalidCommand(cmd);
Potresti presentare un enum. Trasformare ogni enum costante in una fabbrica è prolisso e ha anche dei costi di memoria di runtime. Ma puoi cercare facilmente un enum e quindi usarlo con == o switch.
import xx.example.Command.*;
Command command = Command.valueOf(commandStr);
return
command == START ? new StartCommand (commandLine) :
command == END ? new EndCommand (commandLine) :
new InvalidCommand(commandLine);
Vai con il tuo istinto e rifletti. Tuttavia, in questa soluzione, si presume che l'interfaccia del comando abbia il metodo setCommandString (String s) accessibile, in modo che newInstance sia facilmente utilizzabile. Inoltre, commandMap è qualsiasi mappa con chiavi String (cmd) alle istanze della classe Command a cui corrispondono.
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);
}
}
Hmm, navigando, e mi sono appena imbattuto in questo. Posso ancora commentare?
IMHO non c'è niente sbagliato con il codice di blocco if / else originale. Questo è semplice e la semplicità deve sempre essere la nostra prima chiamata nel design ( http://c2.com/cgi / wiki? DoTheSimplestThingThatCouldPossiblyWork )
Questo sembra vero soprattutto perché tutte le soluzioni offerte sono molto meno autodocumentate rispetto al codice originale ... Voglio dire, non dovremmo scrivere il nostro codice per la lettura piuttosto che per la traduzione ...
Come minimo, il tuo comando dovrebbe avere getCommandString () - dove StartCommand esegue l'override per restituire " START " ;. Quindi puoi semplicemente registrarti o scoprire le lezioni.
+1 sul suggerimento di riflessione, ti darà una struttura più sana nella tua classe.
In realtà potresti fare quanto segue (se non ci hai già pensato) crea metodi corrispondenti alla stringa che ti aspetteresti come argomento per il tuo metodo factory getCommand (), quindi tutto ciò che devi fare è riflettere e invocare () questi metodi e restituire l'oggetto corretto.
