Рефакторинг Java фабричного метода

Question

В этом коде есть что-то очень неудовлетворительное:

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

Я не раскаялся по поводу нескольких точек выхода - структура ясна. Но я не доволен серией почти идентичных утверждений. Я рассмотрел создание карты строк для команд:

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

... затем с помощью Reflection сделать экземпляры соответствующего класса оторванными от карты. Однако, хотя концептуально элегантно, это включает в себя значительное количество кода Reflection, который тот, кто наследует этот код, может не оценить - хотя эта стоимость может быть компенсирована преимуществами. Все строки, жестко кодирующие значения в commandMap, пахнут почти так же плохо, как блок if.

Еще лучше было бы, если бы конструктор фабрики мог сканировать путь к классам для подклассов Command, запрашивать их для представлений String и автоматически добавлять их в свой репертуар.

Итак - как мне провести рефакторинг этого?

Я предполагаю, что некоторые из этих платформ дают мне такие вещи бесплатно. Давайте предположим, что я не в состоянии перенести этот материал в такую ​​среду.

Answer 1

Как насчет следующего кода:

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

valueOf(String) в enum используется для поиска правильного заводского метода. Если фабрика не существует, она выдаст IllegalArgumentException. Мы можем использовать это как сигнал для создания объекта InvalidCommand.

Дополнительным преимуществом является то, что если вы можете сделать метод create(String cmd) общедоступным, если вы также сделаете этот способ построения объекта Command, проверяемого на время компиляции, доступным для остальной части вашего кода. Затем вы можете использовать CommandFactory.START.create(String cmd) для создания объекта Command.

Последнее преимущество заключается в том, что вы легко можете создать список всех доступных команд в документации Javadoc.

Answer 2

Ваша карта строк команд, я думаю, это хорошо. Вы могли бы даже выделить имя строковой команды для конструктора (т.е. не должен ли StartCommand знать, что ее команда «START»?) Если бы вы могли это сделать, создание объектов вашей команды намного проще:

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

Другой вариант - создать enum всех ваших команд со ссылками на классы (предположим, что все ваши объекты команд реализуют 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();
    }
}

, поскольку toString перечисления является его именем, вы можете использовать EnumSet, чтобы найти нужный объект и получить класс изнутри.

Answer 3

За исключением

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

часть, для меня это не так уж плохо. Вы можете использовать Карту и использовать отражение, но, в зависимости от того, как часто вы добавляете / изменяете команды, это может вам не выгодно.

Вы, вероятно, должны задокументировать, почему вам нужны только первые 8 символов, или, возможно, изменить протокол, чтобы легче было определить, какая часть этой строки является командой (например, поставить маркер наподобие «:» или «;» после ключевое слово команды).

Answer 4

Это не является прямым ответом на ваш вопрос, но почему бы вам не выдать InvalidCommandException (или что-то подобное), а не возвращать объект типа InvalidCommand?

Answer 5

Если нет причины, по которой они не могут быть, я всегда стараюсь сделать свои реализации команд без состояния. В этом случае вы можете добавить метод булевого идентификатора метода (String id) в свой командный интерфейс, который сообщит, можно ли использовать этот экземпляр для данного строкового идентификатора. Тогда ваша фабрика может выглядеть примерно так (примечание: я не компилировал и не проверял это):

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

Мне нравится ваша идея, но если вы хотите избежать размышлений, вы можете вместо этого добавить экземпляры в HashMap:

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

Всякий раз, когда вам нужна команда, вы просто клонируете ее:

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

И после этого вы устанавливаете командную строку:

command.setCommandString(cmdName);

Но использование clone () не так элегантно, как отражение: (

Answer 7

Одна опция будет для каждого типа команды иметь свою собственную фабрику. Это дает вам два преимущества:

1) Ваш универсальный завод не назвал бы новый. Таким образом, каждый тип команды может в будущем возвращать объект другого класса в соответствии с аргументами после пробела в строке.

2) В вашей схеме HashMap вы можете избежать отражения путем сопоставления для каждого класса команд объекта, реализующего интерфейс SpecialisedCommandFactory, вместо сопоставления с самим классом. Этот объект на практике, вероятно, будет одноэлементным, но его не нужно указывать как таковой. Затем ваш общий getCommand вызывает специализированный getCommand.

Тем не менее, фабричное распространение может выйти из-под контроля, и код, который у вас есть, является самой простой вещью, которая делает эту работу. Лично я бы, вероятно, оставил все как есть: вы можете сравнивать списки команд в source и spec без нелокальных соображений, таких как то, что раньше называлось CommandFactory.registerCommand, или какие классы были обнаружены с помощью отражения. Это не смущает. Маловероятно, что он будет медленным для менее чем тысячи команд. Единственная проблема заключается в том, что вы не можете добавлять новые типы команд без изменения фабрики. Но модификация, которую вы сделаете, проста и повторяется, и если вы забудете ее сделать, вы получите очевидную ошибку для командных строк, содержащих новый тип, так что это не обременительно.

Answer 8

Другим подходом к динамическому поиску класса для загрузки было бы опустить явную карту и просто попытаться создать имя класса из командной строки. Регистр заглавных букв и алгоритм объединения могут включить «START» -> «com.mypackage.commands.StartCommand» и просто использовать отражение, чтобы попытаться создать его экземпляр. Как-то не получится (экземпляр InvalidCommand или ваше собственное исключение), если вы не можете найти класс.

Затем вы добавляете команды, просто добавляя один объект, и начинаете его использовать.

Answer 9

Использование метода конвекции против конфигурации и использование отражения для поиска доступных объектов Command и загрузки их на карту - это то, что нужно. Затем у вас есть возможность выставлять новые команды без перекомпиляции фабрики.

Answer 10

Я бы предложил избегать рефлексии, если это вообще возможно. Это немного зло.

Вы можете сделать ваш код более кратким, используя троичный оператор:

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

Вы можете ввести перечисление. Создание каждой перечисляемой константы фабрикой является многословным, а также требует некоторой стоимости оперативной памяти. Но вы можете легко найти перечисление, а затем использовать его с == или переключателем.

 import xx.example.Command.*;

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