рефакторинга 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); 
    } 
} 

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

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

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

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

Итак - как мне следует пересмотреть это?

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

Answer 1

Ваша карта строк команд Я думаю, это хорошо. Можно даже вынесет имя команды строки конструктора (? Т.е. не должен знать, что Команда пуска его команда «СТАРТА») Если бы вы могли это сделать, конкретизация ваших командных объектов гораздо проще:

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 2

По крайней мере, ваша команда должна иметь getCommandString() - где StartCommand переопределяет, чтобы вернуть «START». Затем вы можете просто зарегистрировать или открыть классы.

Answer 3

Использование подхода Convetion vs Configuration и использование отражения для сканирования доступных объектов Command и их загрузка на вашу карту. Затем у вас есть возможность открыть новые команды без перекомпиляции фабрики.

Answer 4

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

Answer 5

Наличие этого повторяющегося кода создания объекта, спрятанного на заводе, не так уж плохо. Если это нужно сделать где-то, по крайней мере, все здесь, поэтому я не буду слишком беспокоиться об этом.

Если вы действительно хотите что-то сделать, возможно, зайдите на карту, но настройте ее из файла свойств и постройте карту из этого реквизита.

Не переходя маршрут обнаружения маршрута (о котором я не знаю), вы всегда будете изменять 2 места: писать класс, а затем добавлять сопоставление где-нибудь (фабрика, файл карты или файл свойств).

Answer 6

С исключением

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

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

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

Answer 7

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

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

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

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

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

command.setCommandString(cmdName); 

Но использование clone() звучит не так элегантно, как с помощью отражения :(

Answer 8

Думая об этом, Вы можете создать маленькие классы Инстанцирования, как:

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

Конечно, это добавляет целую кучу, если крошечные классы, которые не могут сделать гораздо больше, чем сказать «да , thats start, дайте мне это «или« нет, вам это не нравится », однако теперь вы можете переделать завод, чтобы содержать список этих CommandCreators и просто спросить каждого из них:« вам нравится эта команда? »и вернуться результат create_instance первого принимающего CommandCreator. Разумеется, теперь выглядит как akward для извлечения первых 8 символов вне CommandCreator, поэтому я бы переработал, чтобы передать всю командную строку в CommandCreator.

Я думаю, что я применил некоторые «Замените переключатель с полиморфизмом» -Редактирование здесь, на случай, если кто-нибудь задается вопросом об этом.

Answer 9

Я бы выбрал карту и создание через отражение. Если сканирование пути к классу происходит слишком медленно, вы всегда можете добавить пользовательскую аннотацию к классу, иметь обработчик аннотации, запущенный во время компиляции, и хранить все имена классов в метаданных jar.

Тогда единственная ошибка, которую вы можете сделать, - это забыть аннотацию.

Я сделал что-то вроде этого некоторое время назад, используя maven и APT.

Answer 10

Если нет причин, по которым они не могут быть, я всегда стараюсь сделать мои реализации команд без гражданства. Если это так, вы можете добавить метод boolean identifier (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 11

Другой подход к динамически найти класс для загрузки, было бы опустить явную карту, а просто попытаться построить имя класса из командной строки. Атрибут заголовка и алгоритм concatenate могут включать «START» -> «com.mypackage.commands.StartCommand» и просто использовать рефлексию, чтобы попытаться создать экземпляр. Если вы не можете найти класс, как-нибудь не получится (InvalidCommand или Exception your own).

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

Answer 12

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

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) из перечисления используется, чтобы найти правильный метод фабрики. Если завод не существует, он выкинет IllegalArgumentException. Мы можем использовать это как сигнал для создания объекта InvalidCommand.

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

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

Answer 13

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

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

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

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

Answer 14

То, как я это делаю, заключается в отсутствии общего метода Factory.

Мне нравится использовать объекты домена как объекты моей команды. Поскольку я использую Spring MVC, это отличный подход, так как DataBinder.setAllowedFields method позволяет мне иметь большую гибкость в использовании одного объекта домена для нескольких разных форм.

Чтобы получить объект команды, у меня есть статический заводский метод в классе объектов Domain. Например, в классе членов у меня были бы такие методы, как:

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

И так далее.

Я не уверен, что это отличный подход, я никогда не видел, чтобы он предлагал где угодно и только что придумал это на моем собственном b/c Мне нравится идея хранить вещи в этом месте в одном месте. Если кто-либо видит какие-либо причины для возражения, пожалуйста, дайте мне знать в комментариях ...

Answer 15

+1 на предложение отражения, это даст вам более разумную структуру в вашем классе.

На самом деле вы можете сделать следующее (если вы еще не подумали об этом) создать методы, соответствующие строкам, которые вы ожидаете в качестве аргумента для вашего метода getCommand(), тогда все, что вам нужно сделать отражает и вызывает() эти методы и возвращает правильный объект.

Answer 16

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

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

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

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

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

Пойдите со своей кишкой и отразите. Однако в этом решении ваш интерфейс Command теперь считается доступным для метода setCommandString (String s), так что newInstance легко можно использовать. Кроме того, commandMap - это любая карта со строковыми ключами (cmd) для экземпляров класса команд, которым они соответствуют.

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

Хм, просмотр и только что натолкнулся на это. Могу ли я еще прокомментировать?

IMHO есть ничего неправильно с исходным кодом if/else. Это просто, и простота всегда должна быть нашим первым вызовом в дизайне (http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork)

Это похоже на то, что все предлагаемые решения гораздо менее самодостаточны, чем исходный код ... Я имею в виду, что мы не должны писать наши код для чтения, а не перевод ...