Refactoring Java Factory-Methode

Question

Es ist etwas sehr unbefriedigend diesen 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);
     }
}

Ich bin unrepentant über die mehrere Ausgangspunkte - die Struktur ist klar. Aber ich bin nicht glücklich über die Serie von nahezu identisch, wenn Aussagen. Ich habe eine Karte von Strings auf Befehle betrachtet machen:

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

... dann Reflexion mit Instanzen der entsprechenden Klasse von der Karte nachgeschlagen zu machen. Allerdings konzeptionell elegant, während beinhaltet dies eine angemessene Menge an Reflexion Code, diesen Code wer erbt schätzen vielleicht nicht - obwohl die Kosten durch die Vorteile ausgeglichen werden könnten. Alle Linien hartzucodieren Werte in die commandMap riechen fast so schlimm wie die if-Block.

Noch besser wäre es, wenn die Konstrukteurs-Fabrik könnte den Classpath für Subklassen von Befehl scannen, fragen sie für String-Darstellungen, und sie automatisch zu seinem Repertoire hinzufügen sie.

So - wie soll ich mich über diese Refactoring

Ich denke, einige der Frameworks da draußen geben Sie mir diese Art der Sache kostenlos. Nehmen wir an, ich bin nicht in der Lage, diese Dinge in einem solchen Rahmen zu migrieren.

Answer 1

Ihre Karte von Strings auf Befehle Ich denke, ist gut. Man könnte sogar die Zeichenfolge Befehlsnamen an den Konstruktor ausklammern (? Das heißt nicht StartCommand wissen, dass sein Befehl „START“) Wenn Sie dies tun könnten, Instanziierung Ihrer Befehlsobjekte ist viel einfacher:

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

Eine andere Möglichkeit ist es, eine enum alle Ihre Befehle mit Links zu den Klassen zu erstellen (nehmen alle Befehlsobjekte implementieren 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();
    }
}

da der toString eines Enum sein Name ist, können Sie EnumSet verwenden Sie das richtige Objekt zu finden und die Klasse erhalten von innen.

Answer 2

Wie wäre es den folgenden 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);
    }
}

Die valueOf(String) der Enumeration wird verwendet, um die richtige Factory-Methode zu finden. Wenn die Fabrik nicht existiert, wird es eine IllegalArgumentException werfen. Wir können dies als Signal verwenden, um das InvalidCommand-Objekt zu erstellen.

Ein zusätzlicher Vorteil ist, dass, wenn Sie die Methode create(String cmd) öffentlich machen können, wenn man auch diese Art der Konstruktion ein Command-Objekt kompiliert Zeit mit dem Rest des Codes überprüft würde. Sie könnten dann verwenden CommandFactory.START.create(String cmd) ein Command-Objekt zu erstellen.

Der letzte Vorteil ist, dass Sie ganz einfach eine Liste aller verfügbaren Befehls in der Javadoc-Dokumentation erstellen können.

Answer 3

Mit Ausnahme der

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

Teil, bedeutet dies nicht so schlecht aus für mich. Mit der Karte und die Verwendung Reflexion gehen könnte, aber, je nachdem, wie oft Sie Erweiterungs- / Änderungsbefehle, könnte dies Ihnen nicht viel kaufen.

Sie sollten wahrscheinlich dokumentieren, warum nur die ersten acht Zeichen wollen, oder vielleicht das Protokoll ändern, so dass es einfacher ist, um herauszufinden, welchen Teil dieser Zeichenfolge ist der Befehl (zB setzte einen Marker wie ‚:‘ oder ‚;‘ nach dem Befehlstaste-Wort).

Answer 4

Es ist nicht direkt eine Antwort auf Ihre Frage, aber warum Sie eine InvalidCommandException (oder so ähnlich) nicht werfen, eher dann ein Objekt vom Typ InvalidCommand Rückkehr?

Answer 5

Es sei denn, es gibt einen Grund ist sie nicht, dass ich immer versuchen, sein kann, um meinen Befehl Implementierungen staatenlos zu machen. Wenn das der Fall ist können Sie eine Methode boolean Bezeichner (String id) Methode, um Ihre Befehlsschnittstelle hinzufügen, die sagen würden, ob diese Instanz für die gegebene Zeichenfolge Kennung verwendet werden könnte. Dann ist Ihre Fabrik könnte in etwa so aussehen (Anmerkung: Ich habe nicht kompilieren oder dies testen):

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

Ich mag Ihre Idee, aber wenn Sie Reflexion vermeiden wollen könnten Sie stattdessen Instanzen der HashMap hinzufügen:

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

Wenn Sie einen Befehl benötigen, können Sie es einfach klonen:

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

Und dann stellen Sie die Befehlsfolge:

command.setCommandString(cmdName);

Aber mit clone () klingt nicht so elegant wie die Reflexion: (

Answer 7

ein Convetion vs Konfiguration Ansatz und die Reflexion zur Verfügung Befehlsobjekte zu scannen und sie in der Karte geladen wäre der Weg zu gehen. Sie haben dann die Möglichkeit, ohne erneute Kompilierung der Fabrik neue Befehle zu belichten.

Answer 8

Ein weiterer Ansatz zur Suche nach dynamisch die Klasse zu laden, wäre die explizite Karte wegzulassen und nur versuchen, den Klassennamen aus der Befehlszeichenfolge zu erstellen. Ein Titel, Fall und verketten Algorithmus könnte sich „START“ -> „com.mypackage.commands.StartCommand“, und nur Reflexion verwenden, um zu versuchen, es zu instanziieren. Scheitern irgendwie (InvalidCommand Instanz oder eine Ausnahme Ihrer eigenen), wenn Sie nicht die Klasse finden kann.

Sie dann Befehle hinzufügen, nur um ein Objekt hinzufügen und starten Sie es.

Answer 9

Eine Option wäre für jeden Befehlstyp sein, seine eigene Fabrik zu haben. Dies gibt Ihnen zwei Vorteile:

1) Ihre allgemeine Fabrik würde nicht neu aufrufen. Also jeder Befehlstyp in Zukunft könnte ein Objekt einer anderen Klasse zurückkehrt nach den Argumenten nach dem Raum Polsterung in der Zeichenkette.

2) In Ihrem HashMap Schema, könnten Sie Reflexion durch, für jede Befehlsklasse, Zuordnung zu einem Objekt einer SpecialisedCommandFactory Schnittstelle implementiert, statt Abbildung auf die Klasse selbst zu vermeiden. Diese Aufgabe in der Praxis wäre wahrscheinlich ein Singleton sein, muß aber nicht als solche angegeben werden. Ihr generisches GetCommand ruft dann das Fach GetCommand.

Wie gesagt, Fabrikproliferation kann aus der Hand, und der Code, den Sie haben, ist die einfachste Sache, die die Arbeit erledigt. Persönlich würde ich es wahrscheinlich lassen, wie es ist: Sie Befehlslisten in Quelle vergleichen und spec ohne nicht-lokale Erwägungen wie das, was vielleicht vorher CommandFactory.registerCommand genannt hat, oder welche Klassen wurde durch Reflexion entdeckt. Es ist nicht verwirrend. Es ist sehr unwahrscheinlich, dass für weniger als tausend Befehle langsam. Das einzige Problem ist, dass Sie keine neue Befehlstypen ohne das Werk zu modifizieren hinzufügen können. Aber die Änderung, die Sie machen würde, ist einfach und repetitiv, und wenn Sie vergessen machen Sie einen offensichtlichen Fehler bekommen für Befehlszeilen den neuen Typ enthält, so ist es nicht schwer.

Answer 10

Mit diesem repetitiven Objekterstellung Code alle in der Fabrik versteckt ist nicht so schlimm. Wenn es irgendwo getan werden muss, zumindest alles hier ist, also würde ich nicht zu viele Sorgen machen.

Wenn Sie wirklich will, etwas dagegen zu tun, gehen Sie vielleicht für die Karte, aber so konfiguriert, dass aus einer Eigenschaftsdatei, und erstellen Sie die Karte aus, dass Datei Requisiten.

Ohne den Classpath Entdeckung Weg zu gehen (über die ich weiß nicht), werden Sie immer 2 Plätze werden zu modifizieren. Eine Klasse zu schreiben, und das Hinzufügen dann eine Zuordnung irgendwo (Fabrik, Karte init oder Properties-Datei)

Answer 11

über das Denken, Sie könnten wenig Instanziierung Klassen erstellen, wie:

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

Natürlich ist dies eine ganze Reihe fügt hinzu, wenn kleine Klassen, die nicht viel mehr tun kann, als sagen: „Ja, das ist zu starten, mir geben, dass“ oder „Nö, nicht so“, jedoch können Sie jetzt überarbeiten die Fabrik eine Liste der CommandCreators enthalten und fragen Sie einfach jede davon: „Sie mögen diesen Befehl“ und gibt das Ergebnis von create_instance der ersten Annahme CommandCreator. Natürlich jetzt ist es Art von akward sieht die ersten 8 Zeichen außerhalb des CommandCreator zu extrahieren, so würde ich überarbeiten, dass so passieren Sie die gesamte Befehlskette in das CommandCreator.

Ich glaube, ich wandte etwas „Ersetzen-Schalter mit Polymorphismus“ hier -Refactoring, falls jemand darüber wundert.

Answer 12

würde ich für die Karte und die Schöpfung durch Reflexion gehen. Wenn das Scannen des Klassenpfades zu langsam ist, können Sie immer eine benutzerdefinierte Anmerkung zur Klasse hinzuzufügen, haben eine Anmerkung Prozessor bei der Kompilierung ausgeführt wird und speichern Sie alle Klassennamen in den JAR-Metadaten.

Dann der einzige Fehler, den Sie tun können, die Anmerkung ist zu vergessen.

Ich habe so etwas wie dies vor einer Weile, mit Maven und APT .

Answer 13

So wie ich es tun, ist nicht eine generische Fabrikmethode haben.

Ich mag Domain-Objekte als meine Befehlsobjekte verwenden. Da ich Spring MVC verwenden ist dies ein großer Ansatz, da die

Ich würde vorschlagen, Reflexion zu vermeiden, wenn überhaupt möglich. Es ist etwas böse.

Sie können den Code kurz unter Verwendung des ternären Operator machen:

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

Sie können eine ENUM vorstellen. jede Enumeration Konstante macht eine Fabrik ausführlich ist und hat auch einige Laufzeitspeicherkosten. Aber Sie können eaily eine ENUM-Lookup und dann verwenden, mit == oder Schalter.

 import xx.example.Command.*;

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

Gehen Sie mit Ihrem Darm, und reflektieren. Doch in dieser Lösung Ihre Kommandoschnittstelle wird nun hat den setCommandString (String s) Verfahren zugänglich angenommen, so dass newInstance leicht verwendbar ist. Außerdem ist commandMap jede Karte mit String-Tasten (cmd) Klasseninstanzen gebieten, daß sie entsprechen.

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, Browsing und nur kam gerade über diese. Kann ich noch sagen?

IMHO gibt es nichts falsch mit dem Original if / else-Block-Code. Dies ist einfach und Einfachheit muss immer unser erster Aufruf im Design sein ( http://c2.com/cgi / wiki? DoTheSimplestThingThatCouldPossiblyWork )

Dies scheint besonders wahr, da alle angebotenen Lösungen viel weniger selbstdokumentiere als der ursprüngliche Code sind ... ich meine, sollten wir nicht unseren Code zum Lesen lieber schreiben als Übersetzung ...

Am allerwenigsten sollte Ihr Befehl ein GetCommandString () - wo StartCommand Vorrang vor zurückzukehren „START“. Dann können Sie einfach registrieren oder entdecken Sie die Klassen.

1 auf dem Reflexions Vorschlag, es wird Ihnen eine vernünftige Struktur in Ihrer Klasse geben.

Eigentlich könnte Sie folgendes tun (wenn Sie nicht darüber nachgedacht haben bereits) create-Methoden zum String, den Sie als Argument für Ihre GetCommand () Factory-Methode erwarten wäre, dann alles, was Sie tun müssen, ist reflektieren und () aufrufen, diese Methoden und senden Sie das richtige Objekt.

Answer 14

Answer 15

Answer 16

Ich würde vorschlagen, Reflexion zu vermeiden, wenn überhaupt möglich. Es ist etwas böse.

Sie können den Code kurz unter Verwendung des ternären Operator machen:

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

Sie können eine ENUM vorstellen. jede Enumeration Konstante macht eine Fabrik ausführlich ist und hat auch einige Laufzeitspeicherkosten. Aber Sie können eaily eine ENUM-Lookup und dann verwenden, mit == oder Schalter.

 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

Gehen Sie mit Ihrem Darm, und reflektieren. Doch in dieser Lösung Ihre Kommandoschnittstelle wird nun hat den setCommandString (String s) Verfahren zugänglich angenommen, so dass newInstance leicht verwendbar ist. Außerdem ist commandMap jede Karte mit String-Tasten (cmd) Klasseninstanzen gebieten, daß sie entsprechen.

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

Gehen Sie mit Ihrem Darm, und reflektieren. Doch in dieser Lösung Ihre Kommandoschnittstelle wird nun hat den setCommandString (String s) Verfahren zugänglich angenommen, so dass newInstance leicht verwendbar ist. Außerdem ist commandMap jede Karte mit String-Tasten (cmd) Klasseninstanzen gebieten, daß sie entsprechen.

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 19

Hmm, Browsing und nur kam gerade über diese. Kann ich noch sagen?

IMHO gibt es nichts falsch mit dem Original if / else-Block-Code. Dies ist einfach und Einfachheit muss immer unser erster Aufruf im Design sein ( http://c2.com/cgi / wiki? DoTheSimplestThingThatCouldPossiblyWork )

Dies scheint besonders wahr, da alle angebotenen Lösungen viel weniger selbstdokumentiere als der ursprüngliche Code sind ... ich meine, sollten wir nicht unseren Code zum Lesen lieber schreiben als Übersetzung ...

Answer 20

Answer 21

Hmm, Browsing und nur kam gerade über diese. Kann ich noch sagen?

Answer 22

IMHO gibt es nichts falsch mit dem Original if / else-Block-Code. Dies ist einfach und Einfachheit muss immer unser erster Aufruf im Design sein (

Answer 23

Am allerwenigsten sollte Ihr Befehl ein GetCommandString () - wo StartCommand Vorrang vor zurückzukehren „START“. Dann können Sie einfach registrieren oder entdecken Sie die Klassen.

Answer 24

1 auf dem Reflexions Vorschlag, es wird Ihnen eine vernünftige Struktur in Ihrer Klasse geben.

Eigentlich könnte Sie folgendes tun (wenn Sie nicht darüber nachgedacht haben bereits) create-Methoden zum String, den Sie als Argument für Ihre GetCommand () Factory-Methode erwarten wäre, dann alles, was Sie tun müssen, ist reflektieren und () aufrufen, diese Methoden und senden Sie das richtige Objekt.