Refactoriser la méthode de la fabrique Java

Question

Il y a quelque chose de très insatisfaisant dans ce 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);
     }
}

Je suis impénitent face aux multiples points de sortie - la structure est claire. Mais je ne suis pas content de la série de déclarations quasi identiques. J'ai envisagé de créer une carte des chaînes en commandes:

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

... puis en utilisant Reflection pour créer des occurrences de la classe appropriée dans la carte. Cependant, bien que conceptuellement élégant, cela implique une bonne quantité de code Reflection que quiconque héritera de ce code pourrait ne pas apprécier - bien que ce coût puisse être compensé par les avantages. Toutes les lignes de valeurs codées en dur dans le commandMap sentent presque aussi mauvais que le bloc if.

Encore mieux, si le constructeur de l'usine pouvait analyser le chemin d'accès aux classes pour rechercher les sous-classes de Command, les interroger pour obtenir des représentations de chaîne et les ajouter automatiquement à son répertoire.

Alors, comment dois-je procéder pour le refactoriser?

Je suppose que certains des cadres existants me donnent ce genre de choses gratuitement. Supposons que je ne suis pas en mesure de migrer ce genre de choses dans un tel cadre.

Answer 1

Votre carte des chaînes de commandes aux commandes est bonne, à mon avis. Vous pouvez même inclure le nom de la commande de chaîne dans le constructeur (par exemple, si StartCommand ne doit pas savoir que sa commande est "START"), l'instanciation de vos objets de commande est beaucoup plus simple:

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

Une autre option consiste à créer un enum de toutes vos commandes avec des liens vers les classes (en supposant que tous vos objets de commande implémentent 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();
    }
}

comme la toString d'une énumération est son nom, vous pouvez utiliser EnumSet pour localiser le bon objet et obtenir la classe de l'intérieur.

Answer 2

Que diriez-vous du code suivant:

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

Le valueOf (String) de l'énum est utilisé pour trouver la méthode d'usine correcte. Si la fabrique n'existe pas, une IllegalArgumentException sera générée. Nous pouvons l'utiliser comme signal pour créer l'objet InvalidCommand .

Un avantage supplémentaire est que si vous pouvez rendre la méthode create (String cmd) publique, vous pouvez également rendre cette manière de construire un objet de commande Heure de compilation cochée vérifiée pour le reste de votre code. Vous pouvez ensuite utiliser CommandFactory.START.create (String cmd ) pour créer un objet Command.

Le dernier avantage est que vous pouvez facilement créer une liste de toutes les commandes disponibles dans votre documentation Javadoc.

Answer 3

À l'exception du

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

En partie, cela ne me semble pas trop mal. Vous pouvez utiliser la carte et utiliser la réflexion, mais selon la fréquence à laquelle vous ajoutez / modifiez des commandes, cela ne vous achètera peut-être pas beaucoup.

Vous devriez probablement indiquer pourquoi vous ne voulez que les 8 premiers caractères, ou peut-être modifier le protocole afin de déterminer plus facilement la partie de cette chaîne qui est la commande (par exemple, placez un marqueur du type ':' ou ';' après mot-clé de commande).

Answer 4

Ce n'est pas directement une réponse à votre question, mais pourquoi ne pas lancer une exception InvalidCommandException (ou quelque chose de similaire) plutôt que de renvoyer un objet de type InvalidCommand?

Answer 5

À moins qu'il y ait une raison pour laquelle ils ne peuvent pas l'être, j'essaie toujours de rendre mes implémentations de commande sans état. Si tel est le cas, vous pouvez ajouter une méthode, identificateur booléen (String id) à votre interface de commande, qui indiquerait si cette instance peut être utilisée pour l'identificateur de chaîne donné. Votre usine pourrait alors ressembler à quelque chose comme ça (note: je n’ai pas compilé ou testé cela):

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

J'aime votre idée, mais si vous souhaitez éviter toute réflexion, vous pouvez ajouter des instances à la HashMap:

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

Chaque fois que vous avez besoin d'une commande, il vous suffit de la cloner:

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

Et ensuite, vous définissez la chaîne de commande:

command.setCommandString(cmdName);

Mais utiliser clone () ne sonne pas aussi élégant que d'utiliser la réflexion: (

Answer 7

Adopter une approche Convetion vs Configuration et utiliser la réflexion pour rechercher les objets Command disponibles et les charger dans votre carte serait la solution. Vous avez ensuite la possibilité d’exposer de nouvelles commandes sans recompiler l’usine.

Answer 8

Une autre approche pour rechercher de manière dynamique la classe à charger consiste à omettre la mappe explicite et à essayer simplement de construire le nom de la classe à partir de la chaîne de commande. Un cas de titre et un algorithme de concaténation pourraient transformer "START". - > "com.mypackage.commands.StartCommand", et utilisez simplement la réflexion pour essayer de l'instancier. En cas d'échec (instance InvalidCommand ou une exception de votre choix) si vous ne trouvez pas la classe.

Ensuite, vous ajoutez des commandes simplement en ajoutant un objet et commencez à l'utiliser.

Answer 9

Une option serait que chaque type de commande ait sa propre fabrique. Cela vous donne deux avantages:

1) Votre usine générique ne s’appellera pas nouvelle. Ainsi, chaque type de commande pourrait à l'avenir renvoyer un objet d'une classe différente en fonction des arguments qui suivent le remplissage d'espace dans la chaîne.

2) Dans votre schéma HashMap, vous pouvez éviter les réflexions en mappant chaque objet de la commande sur un objet implémentant une interface SpecialisedCommandFactory au lieu de le mapper sur la classe elle-même. En pratique, cet objet serait probablement un singleton, mais n'a pas besoin d'être spécifié comme tel. Votre commande générique getCommand appelle ensuite la commande spécialisée get.

Cela dit, la prolifération des usines peut devenir incontrôlable et le code que vous avez est la chose la plus simple qui fait le travail. Personnellement, je le laisserais probablement tel quel: vous pouvez comparer les listes de commandes source et spec sans considérations non locales, comme ce qui aurait pu s'appeler auparavant CommandFactory.registerCommand, ou les classes découvertes par réflexion. Ce n'est pas déroutant. Il est très peu probable qu’il soit lent pour moins de mille commandes. Le seul problème est que vous ne pouvez pas ajouter de nouveaux types de commandes sans modifier la fabrique. Cependant, la modification que vous apportez est simple et répétitive. Si vous oubliez de le faire, vous obtenez une erreur évidente pour les lignes de commande contenant le nouveau type. Ce n'est donc pas onéreux.

Answer 10

Avoir ce code de création d'objet répétitif tout caché dans la fabrique n'est pas si grave. Si cela doit être fait quelque part, au moins tout est là, donc je ne m'inquiéterais pas trop à ce sujet.

Si vous voulez vraiment, faire quelque chose à ce sujet, optez peut-être pour la carte, mais configurez-la à partir d'un fichier de propriétés et construisez la carte à partir de ce fichier d'accessoire.

Sans passer par la route de découverte des chemins de classes (ce que je ne connais pas), vous allez toujours modifier 2 emplacements: écrire une classe, puis ajouter un mappage quelque part (fabrique, init de carte ou fichier de propriétés).

Answer 11

En réfléchissant à cela, vous pouvez créer de petites classes d'instanciation, telles que:

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

Bien sûr, cela ajoute tout un tas de choses si de petites classes qui ne peuvent pas faire plus que de dire "oui, ça commence, donnez-moi ça" ou "Nope, n'aime pas ça", mais vous pouvez maintenant retravailler l’usine pour qu'elle contienne une liste de ces CommandCreators et demandez à chacune d’elles: "vous aimez cette commande?" et retourne le résultat de create_instance du premier CommandCreator qui accepte. Bien sûr, il est maintenant assez difficile d'extraire les 8 premiers caractères en dehors de CommandCreator, aussi je retravaillerais cela pour que vous transmettiez toute la chaîne de commande dans CommandCreator.

Je pense avoir appliqué un "Remplacer le commutateur par un polymorphisme" ici, au cas où quelqu'un s'interrogerait à ce sujet.

Answer 12

Je choisirais la carte et la création par réflexion. Si l'analyse du chemin de classe est trop lente, vous pouvez toujours ajouter une annotation personnalisée à la classe, faire exécuter un processeur d'annotation au moment de la compilation et stocker tous les noms de classe dans les métadonnées de jar.

Ensuite, la seule erreur que vous puissiez faire est d'oublier l'annotation.

J'ai fait quelque chose comme cela il y a quelque temps, en utilisant maven et APT .

Answer 13

La façon dont je le fais est de ne pas avoir de méthode Factory générique.

J'aime utiliser les objets de domaine comme objets de commande. Depuis que j'utilise Spring MVC, c’est une excellente approche car La méthode DataBinder.setAllowedFields me permet beaucoup de flexibilité pour utiliser un seul objet de domaine pour plusieurs formulaires différents.

Pour obtenir un objet de commande, j'ai une méthode de fabrique statique sur la classe d'objet Domain. Par exemple, dans la classe de membre, j'aurais des méthodes comme -

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

Et ainsi de suite.

Je ne suis pas sûr qu'il s'agisse d'une bonne approche, je ne l'ai jamais vue suggérée nulle part et je l'ai simplement imaginé moi-même, mais j'aime bien l'idée de garder de telles choses au même endroit. Si quelqu'un voit une raison de s'opposer, faites-le-moi savoir dans les commentaires ...

Answer 14

Je suggérerais d'éviter si possible la réflexion. C'est un peu mauvais.

Vous pouvez rendre votre code plus concis en utilisant l'opérateur ternaire:

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

Vous pouvez introduire une énumération. Rendre chaque énumération constante, une fabrique est prolixe et présente également un coût en mémoire d'exécution. Mais vous pouvez facilement rechercher une énumération, puis l’utiliser avec == ou basculer.

 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

Allez avec vos tripes et réfléchissez. Toutefois, dans cette solution, votre interface de commande est maintenant supposée avoir la méthode setCommandString (String s) accessible, de sorte que newInstance est facilement utilisable. CommandMap correspond également à toute mappe avec des clés String (cmd) aux instances de la classe Command auxquelles elles correspondent.

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, en train de naviguer, et je viens juste de trouver ça. Puis-je quand même commenter?

IMHO, il n'y a rien avec le code de blocage if / else d'origine. C’est simple et la simplicité doit toujours être notre premier appel dans la conception ( http://c2.com/cgi / wiki? DoTheSimplestThingThatCouldPossiblyWork )

Cela semble particulièrement vrai, car toutes les solutions proposées sont beaucoup moins auto-documentées que le code original ... Je veux dire, ne devrions-nous pas écrire notre code pour la lecture plutôt que pour la traduction ...

Answer 17

À tout le moins, votre commande devrait avoir la propriété getCommandString () - où StartCommand remplace le mot de passe "START". Ensuite, vous pouvez simplement vous inscrire ou découvrir les cours.

Answer 18

+1 sur la suggestion de réflexion, cela vous donnera une structure plus saine dans votre classe.

En fait, vous pouvez faire ce qui suit (si vous n'y avez pas déjà pensé) Créez des méthodes correspondant à la chaîne que vous attendriez en tant qu'argument de votre méthode de fabrique getCommand (). Il vous suffira alors de réfléchir et d'appeler () ces méthodes et de renvoyer l'objet correct.