15

这段代码有一些非常不满意的地方：

/*
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.

...然后使用反射使适当类的实例从地图中查找。然而，虽然在概念上很优雅，但这涉及到相当多的反射代码，继承此代码的人可能不会欣赏 - 尽管该成本可能会被收益所抵消。将值硬编码到 commandMap 中的所有行的气味几乎与 if 块一样糟糕。

如果工厂的构造函数可以扫描类路径以查找 Command 的子类，查询它们以获取字符串表示形式，并自动将它们添加到其曲目中，那就更好了。

那么 - 我应该如何去重构呢？

我想那里的一些框架免费给了我这种东西。假设我无法将这些东西迁移到这样的框架中。

4

18 回答 18

14

下面的代码怎么样：

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)公开，如果您还可以使这种构造命令对象的方式编译时间检查可用于您的其余代码。然后，您可以使用CommandFactory.START.create(String cmd) 创建一个 Command 对象。

最后一个好处是您可以轻松地在 Javadoc 文档中创建所有可用命令的列表。

于 2008-09-22T15:42:50.867 回答
12

我认为你的字符串到命令的映射很好。您甚至可以将字符串命令名称分解为构造函数（即 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来定位正确的对象并从内部获取类。

于 2008-09-22T14:58:14.203 回答
4

除了

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

部分，这对我来说看起来还不错。您可以使用 Map 并使用反射，但是，根据您添加/更改命令的频率，这可能不会给您带来太多收益。

您可能应该记录为什么只需要前 8 个字符，或者更改协议以便更容易确定该字符串的哪一部分是命令（例如，在命令键之后放置一个标记，如 ':' 或 ';'单词）。

于 2008-09-22T15:00:57.160 回答
3

它不是您问题的直接答案，但您为什么不抛出 InvalidCommandException（或类似的东西），而不是返回 InvalidCommand 类型的对象？

于 2008-09-22T14:55:50.043 回答
3

除非有理由他们不能是我总是试图让我的命令实现无状态。如果是这种情况，您可以在命令接口中添加一个方法 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);
    }
}
于 2008-09-22T15:03:49.913 回答
2

我喜欢你的想法，但如果你想避免反射，你可以向 HashMap 添加实例：

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

每当您需要命令时，只需克隆它：

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

然后，您设置命令字符串：

command.setCommandString(cmdName);

但是使用 clone() 听起来不像使用反射那么优雅:(

于 2008-09-22T15:01:39.107 回答
1

采用 Convetion vs Configuration 方法并使用反射来扫描可用的 Command 对象并将它们加载到您的地图中将是可行的方法。然后，您可以在不重新编译工厂的情况下公开新命令。

于 2008-09-22T14:55:26.050 回答
1

另一种动态查找要加载的类的方法是省略显式映射，而只是尝试从命令字符串构建类名。标题案例和连接算法可以变成“START”->“com.mypackage.commands.StartCommand”，并且只需使用反射来尝试实例化它。如果找不到类，则以某种方式失败（InvalidCommand 实例或您自己的异常）。

然后，您只需添加一个对象即可添加命令并开始使用它。

于 2008-09-22T15:04:50.193 回答
1

一种选择是让每种命令类型都有自己的工厂。这给你两个好处：

1）您的通用工厂不会调用新的。因此，每种命令类型将来都可以根据字符串中空格填充后的参数返回不同类的对象。

2）在您的 HashMap 方案中，您可以通过将每个命令类映射到实现 SpecialisedCommandFactory 接口的对象而不是映射到类本身来避免反射。这个对象在实践中可能是一个单例，但不需要这样指定。然后，您的通用 getCommand 调用专用的 getCommand。

也就是说，工厂扩散可能会失控，而您拥有的代码是完成这项工作的最简单的东西。就我个人而言，我可能会保持原样：您可以比较源代码和规范中的命令列表，而无需考虑以前可能称为 CommandFactory.registerCommand 的非本地考虑，或者通过反射发现了哪些类。这并不令人困惑。对于少于一千个命令，它不太可能很慢。唯一的问题是你不能在不修改工厂的情况下添加新的命令类型。但是您所做的修改是简单且重复的，如果您忘记进行修改，您会在包含新类型的命令行中得到明显的错误，因此它并不繁重。

于 2008-09-22T15:43:06.637 回答
0

将这种重复的对象创建代码全部隐藏在工厂中并不是那么糟糕。如果它必须在某个地方完成，至少它都在这里，所以我不会太担心它。

如果你真的想对它做点什么，也许去地图，但从一个属性文件配置它，然后从那个道具文件构建地图。

如果不走类路径发现路线（我不知道），您将始终修改 2 个地方：编写一个类，然后在某处添加映射（工厂、映射初始化或属性文件）。

于 2008-09-22T14:59:03.000 回答
0

考虑到这一点，您可以创建小的实例化类，例如： 

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

当然，如果小类只能说“是的，那是开始，给我那个”或“不，不喜欢那个”，那么这会增加一大堆，但是，您现在可以将工厂重新设计为包含这些 CommandCreators 的列表，然后问每个人：“你喜欢这个命令吗？” 并返回第一个接受 CommandCreator 的 create_instance 的结果。当然，现在提取 CommandCreator 之外的前 8 个字符看起来有点不妥，所以我会修改它，以便将整个命令字符串传递给 CommandCreator。

我想我在这里应用了一些“用多态性替换开关”-重构，以防有人对此感到疑惑。

于 2008-09-22T15:02:03.570 回答
0

我会通过反射去制作地图和创作。如果扫描类路径太慢，您可以随时向类添加自定义注释，在编译时运行注释处理器并将所有类名存储在 jar 元数据中。

然后，您唯一能做的就是忘记注释。

不久前我做了类似的事情，使用 maven 和APT。

于 2008-09-22T15:02:20.400 回答
0

我这样做的方法是没有通用的工厂方法。

我喜欢使用域对象作为我的命令对象。由于我使用 Spring MVC，这是一个很好的方法，因为DataBinder.setAllowedFields 方法允许我非常灵活地将单个域对象用于几种不同的形式。

要获取命令对象，我在 Domain 对象类上有一个静态工厂方法。例如，在成员类中，我会有类似的方法 -

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

等等。

我不确定这是一个很好的方法，我从来没有在任何地方看到过它的建议，只是我自己想出了它，我喜欢把这样的东西放在一个地方的想法。如果有人看到任何反对的理由，请在评论中告诉我...

于 2008-09-22T15:51:30.440 回答
0

如果可能的话，我建议避免反思。这有点邪恶。

您可以使用三元运算符使代码更简洁：

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

你可以引入一个枚举。使每个枚举常量成为工厂是冗长的，并且还具有一些运行时内存成本。但是您可以很容易地查找一个枚举，然后将其与 == 或 switch 一起使用。

 import xx.example.Command.*;

 Command command = Command.valueOf(commandStr);
 return 
     command == START ? new StartCommand  (commandLine) :
     command == END   ? new EndCommand    (commandLine) :
                        new InvalidCommand(commandLine);
于 2008-09-22T16:00:51.773 回答
0

跟着你的直觉走，然后反思。但是，在此解决方案中，现在假定您的 Command 接口具有可访问的 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);
     }
}
于 2008-09-22T23:54:06.973 回答
0

嗯，浏览，才刚刚遇到这个。我还能评论吗？

恕我直言，原始 if/else 块代码没有任何问题。这很简单，简单必须始终是我们在设计中的第一个要求（http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork）

这似乎是真的，因为所提供的所有解决方案都比原始代码更少自我记录......我的意思是我们不应该编写我们的代码来阅读而不是翻译......

于 2010-06-14T14:03:47.467 回答
-1

至少，您的命令应该有一个 getCommandString() - 其中 StartCommand 覆盖以返回“START”。然后，您可以注册或发现课程。

于 2008-09-22T14:54:52.773 回答
-1

+1 关于反思建议，它将为您的班级提供更健全的结构。

实际上，您可以执行以下操作（如果您还没有考虑过）创建与您期望的 String 对应的方法作为 getCommand() 工厂方法的参数，那么您所要做的就是反射和调用（ ) 这些方法并返回正确的对象。

于 2008-09-22T15:56:41.270 回答