Рефакторинг фабричного метода Java
https://stackoverflow.com/questions/115269
italiano
english
français
española
中国
日本の
العربية
Deutsch
한국어
Português
RussianВопрос
В этом коде есть что-то очень неудовлетворительное:
/*
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.
...затем используя Reflection, чтобы экземпляры соответствующего класса искались на карте.Однако, несмотря на концептуальную элегантность, это требует изрядного количества кода Reflection, который тот, кто унаследует этот код, может не оценить - хотя эти затраты могут быть компенсированы преимуществами.Все значения строк, жестко запрограммированные в файле CommandMap, пахнут почти так же плохо, как и блок if.
Еще лучше было бы, если бы конструктор фабрики мог сканировать путь к классам на предмет подклассов Command, запрашивать у них строковые представления и автоматически добавлять их в свой репертуар.
Итак, как мне следует провести рефакторинг?
Я думаю, что некоторые из существующих фреймворков предоставляют мне подобные вещи бесплатно.Давайте предположим, что я не в состоянии перенести все это в такую структуру.
Решение
Я считаю, что ваша карта строк с командами хороша.Вы даже можете выделить имя строковой команды в конструктор (т.е.разве StartCommand не должен знать, что его команда — «СТАРТ»?) Если бы вы могли это сделать, создание экземпляров ваших командных объектов было бы намного проще:
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 чтобы найти нужный объект и получить класс изнутри.
Другие советы
Как насчет следующего кода:
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) public, если вы также сделаете этот способ создания объекта Command с проверкой времени компиляции доступным для остальной части вашего кода.Затем вы можете использовать CommandFactory.START.create(String cmd), чтобы создать объект Command.
Последнее преимущество заключается в том, что вы можете легко создать список всех доступных команд в документации Javadoc.
За исключением
cmd.subString(0,8).trim();
часть, это не выглядит так уж плохо для меня.Вы можете использовать Карту и отражение, но, в зависимости от того, как часто вы добавляете/изменяете команды, это может не принести вам много пользы.
Вероятно, вам следует задокументировать, почему вам нужны только первые 8 символов, или, возможно, изменить протокол, чтобы было легче определить, какая часть этой строки является командой (например,Положите маркер, подобный ':' или ';' После командного ключа).
Это не прямой ответ на ваш вопрос, но почему бы вам не вызвать InvalidCommandException (или что-то подобное), а не возвращать объект типа InvalidCommand?
Если нет причины, по которой это невозможно, я всегда стараюсь сделать реализацию своих команд без сохранения состояния.В этом случае вы можете добавить метод логического идентификатора (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);
}
}
Мне нравится ваша идея, но если вы хотите избежать отражения, вы можете вместо этого добавить экземпляры в HashMap:
commandMap = new HashMap();
commandMap.put("START",new StartCommand());
Всякий раз, когда вам нужна команда, вы просто клонируете ее:
command = ((Command) commandMap.get(cmdName)).clone();
И после этого вы устанавливаете командную строку:
command.setCommandString(cmdName);
Но использование clone() звучит не так элегантно, как использование отражения :(
Лучше всего было бы использовать подход Convetion vs Configuration и использовать отражение для сканирования доступных объектов Command и загрузки их на карту.После этого у вас появится возможность предоставлять новые команды без перекомпиляции фабрики.
Другой подход к динамическому поиску загружаемого класса заключается в том, чтобы пропустить явное сопоставление и просто попытаться создать имя класса из командной строки.Регистр заголовка и алгоритм объединения могут превратить «START» -> «com.mypackage.commands.StartCommand» и просто использовать отражение, чтобы попытаться создать его экземпляр.По какой-то причине произойдет сбой (экземпляр InvalidCommand или собственное исключение), если вы не можете найти класс.
Затем вы добавляете команды, просто добавляя один объект, и начинаете его использовать.
Одним из вариантов может быть создание для каждого типа команды собственной фабрики.Это дает вам два преимущества:
1) Ваш универсальный завод не будет называться новым.Таким образом, каждый тип команды может в будущем возвращать объект другого класса в соответствии с аргументами, следующими за пробелами в строке.
2) В вашей схеме HashMap вы можете избежать отражения, для каждого класса команд сопоставляя его с объектом, реализующим интерфейс SpecializedCommandFactory, вместо сопоставления с самим классом.На практике этот объект, вероятно, будет одноэлементным, но его не обязательно указывать как таковой.Затем ваш общий метод getCommand вызывает специализированный метод getCommand.
Тем не менее, распространение фабрик может выйти из-под контроля, и имеющийся у вас код — это самое простое, что выполняет эту работу.Лично я бы, наверное, оставил как есть:вы можете сравнивать списки команд в исходном коде и спецификации, не принимая во внимание нелокальные соображения, например то, что раньше могло называться CommandFactory.registerCommand, или какие классы были обнаружены посредством отражения.Это не сбивает с толку.Маловероятно, что он будет медленным менее чем для тысячи команд.Единственная проблема заключается в том, что вы не можете добавлять новые типы команд без изменения фабрики.Но вносимые вами изменения просты и повторяются, и если вы забудете их внести, вы получите очевидную ошибку для командных строк, содержащих новый тип, так что это не обременительно.
Спрятать этот повторяющийся код создания объекта в фабрике не так уж и плохо.Если это нужно где-то сделать, по крайней мере, все это здесь, так что я бы не особо об этом беспокоился.
Если вы Действительно хотите что-то с этим сделать, возможно, выберите карту, но настройте ее из файла свойств и создайте карту из этого файла реквизита.
Не идя по маршруту обнаружения пути к классам (о котором я не знаю), вы всегда будете изменять 2 места:написание класса, а затем добавление куда-либо сопоставления (фабрика, инициализация карты или файл свойств).
Подумав об этом, вы можете создать небольшие классы создания экземпляров, например:
class CreateStartCommands implements CommandCreator {
public bool is_fitting_commandstring(String identifier) {
return identifier == "START"
}
public Startcommand create_instance(cmd) {
return StartCommand(cmd);
}
}
Конечно, это добавляет целую кучу крошечных классов, которые не могут ничего сделать, кроме как сказать «да, это начало, дай мне это» или «нет, мне это не нравится», однако теперь вы можете переделать фабрику, чтобы содержать список этих CommandCreators и просто задавать вопросы каждому из них:"Тебе нравится эта команда?" и вернуть результат create_instance первого принятия CommandCreator.Конечно, сейчас выглядит несколько неудобно извлекать первые 8 символов за пределы CommandCreator, поэтому я бы переработал это, чтобы вы передавали всю командную строку в CommandCreator.
Я думаю, что я применил здесь некоторый рефакторинг «Заменить переключатель полиморфизмом», на случай, если кто-то задумается об этом.
Я бы выбрал карту и создание через отражение.Если сканирование пути к классу выполняется слишком медленно, вы всегда можете добавить к классу собственную аннотацию, запустить обработчик аннотаций во время компиляции и сохранить все имена классов в метаданных jar.
Тогда единственная ошибка, которую вы можете сделать, — это забыть аннотацию.
Я сделал что-то подобное некоторое время назад, используя maven и АПТ.
Я делаю это так, чтобы не иметь общего фабричного метода.
Мне нравится использовать объекты домена в качестве объектов команд.Поскольку я использую Spring MVC, это отличный подход, поскольку Метод DataBinder.setAllowedFields дает мне большую гибкость в использовании одного объекта предметной области для нескольких различных форм.
Чтобы получить объект команды, у меня есть статический фабричный метод в классе объектов Domain.Например, в классе-члене у меня были бы такие методы, как -
public static Member getCommandObjectForRegistration();
public static Member getCommandObjectForChangePassword();
И так далее.
Я не уверен, что это отличный подход, я никогда не видел, чтобы он предлагался где-либо, и вроде как придумал его сам, потому что мне нравится идея хранить подобные вещи в одном месте.Если кто-то видит причины возражать, пожалуйста, дайте мне знать в комментариях...
Я бы посоветовал избегать размышлений, если это вообще возможно.Это в некоторой степени зло.
Вы можете сделать свой код более кратким, используя тернарный оператор:
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);
Следуйте своей интуиции и подумайте.Однако в этом решении теперь предполагается, что ваш командный интерфейс имеет доступный метод setCommandString(String s), так что newInstance можно легко использовать.Кроме того, CommandMap — это любое сопоставление строковых ключей (cmd) с экземплярами класса Command, которым они соответствуют.
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);
}
}
Хм, просматривал и только что наткнулся на это.Могу ли я еще прокомментировать?
ИМХО есть ничего неправильно с исходным кодом блока if/else.Это просто, и простота всегда должна быть нашим первым требованием в дизайне (http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossibleWork)
Это кажется особенно верным, поскольку все предлагаемые решения гораздо менее самодокументированы, чем исходный код... Я имею в виду, не следует ли нам писать наш код для чтения, а не для перевода...
По крайней мере, ваша команда должна иметь getCommandString(), где StartCommand переопределяет возврат «START».Затем вы можете просто зарегистрироваться или открыть для себя классы.
+1 к предложению по размышлению, это даст вам более разумную структуру в вашем классе.
На самом деле вы могли бы сделать следующие (если вы еще не подумали об этом) создать методы, соответствующие строке, которую вы бы ожидали в качестве аргумента для вашего метода заводской фабрики getCommand (), то все, что вам нужно сделать, это отразить и вызвать ( ) эти методы и вернуть правильный объект.
