java - 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.

...次に、リフレクションを使用して、マップからルックアップされた適切なクラスのインスタンスを作成します。ただし、概念的には洗練されていますが、これにはかなりの量の Reflection コードが含まれており、このコードを継承する人は誰でも理解できない可能性があります。ただし、そのコストは利点によって相殺される可能性があります。commandMap に値をハードコーディングするすべての行は、if ブロックとほぼ同じくらい悪臭を放ちます。

ファクトリのコンストラクターが Command のサブクラスのクラスパスをスキャンし、それらに文字列表現を照会し、それらをレパートリーに自動的に追加できれば、さらに良いでしょう。

では、これをリファクタリングするにはどうすればよいですか?

そこにあるフレームワークの中には、この種のものを無料で提供してくれるものがあると思います。私がこのようなものをそのようなフレームワークに移行する立場にないと仮定しましょう。

score 14 · Accepted Answer

次のコードはどうでしょう。

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 ドキュメントで使用可能なすべてのコマンドのリストを簡単に作成できることです。

score 12 · Accepted Answer

文字列からコマンドへのマップは良いと思います。文字列コマンド名をコンストラクターに渡すこともできます (つまり、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ため、適切なオブジェクトを見つけてその中からクラスを取得するために使用できます。

score 4 · Accepted Answer

を除いて、

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

一部、これは私にはそれほど悪くはありません。マップを使用してリフレクションを使用することもできますが、コマンドを追加/変更する頻度によっては、あまり効果がない場合があります。

最初の 8 文字だけが必要な理由を文書化するか、プロトコルを変更して、その文字列のどの部分がコマンドであるかを簡単に把握できるようにする必要があります (たとえば、コマンドキーの後に ':' や ';' などのマーカーを配置します)。語）。

score 3 · Accepted Answer

理由がない限り、私は常にコマンドの実装をステートレスにしようとしています。その場合は、コマンドインターフェイスにメソッドブール識別子（文字列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);
    }
}

score 3 · Accepted Answer

あなたの質問に対する直接の回答ではありませんが、InvalidCommand 型のオブジェクトを返すのではなく、InvalidCommandException (または同様のもの) をスローしないのはなぜですか?

score 2 · Accepted Answer

私はあなたのアイデアが好きですが、リフレクションを避けたい場合は、代わりに HashMap にインスタンスを追加できます:

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

コマンドが必要なときはいつでも、それを複製するだけです：

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

その後、コマンド文字列を設定します。

command.setCommandString(cmdName);

しかし、 clone() の使用は、リフレクションの使用ほどエレガントに聞こえません:(

score 1 · Accepted Answer

ロードするクラスを動的に見つける別のアプローチは、明示的なマップを省略し、コマンド文字列からクラス名を作成することです。タイトルケースと連結アルゴリズムは、「START」->「com.mypackage.commands.StartCommand」に変わり、リフレクションを使用してインスタンス化を試みることができます。クラスが見つからない場合は、何らかの理由で失敗します（InvalidCommandインスタンスまたは独自の例外）。

次に、オブジェクトを1つ追加するだけでコマンドを追加し、使用を開始します。

score 1 · Accepted Answer

Convetion と Configuration のアプローチを採用し、リフレクションを使用して利用可能な Command オブジェクトをスキャンし、それらをマップにロードする方法が適しています。その後、ファクトリを再コンパイルせずに新しいコマンドを公開できます。

score 1 · Accepted Answer

1 つのオプションは、コマンドタイプごとに独自のファクトリを持つことです。これにより、次の 2 つの利点が得られます。

1) ジェネリックファクトリは new を呼び出しません。したがって、各コマンドタイプは、文字列内のスペースパディングに続く引数に従って、異なるクラスのオブジェクトを返す可能性があります。

2) HashMap スキームでは、コマンドクラスごとに、クラス自体にマッピングするのではなく、SpecializedCommandFactory インターフェイスを実装するオブジェクトにマッピングすることで、リフレクションを回避できます。実際には、このオブジェクトはおそらくシングルトンですが、そのように指定する必要はありません。次に、一般的な getCommand が特殊化された getCommand を呼び出します。

そうは言っても、工場の急増は手に負えなくなる可能性があり、あなたが持っているコードはその仕事をする最も単純なものです. 個人的には、おそらくそのままにしておきます。以前に CommandFactory.registerCommand と呼ばれていたものや、リフレクションによって発見されたクラスなど、ローカル以外の考慮事項なしで、ソースと仕様のコマンドリストを比較できます。混乱しません。1,000 未満のコマンドの場合、遅くなる可能性はほとんどありません。唯一の問題は、ファクトリを変更しないと新しいコマンドタイプを追加できないことです。しかし、行う変更は単純で反復的であり、変更を忘れると、新しい型を含むコマンドラインに対して明らかなエラーが発生するため、面倒ではありません。

score 0 · Accepted Answer

可能な限り、反省を避けることをお勧めします。やや悪です。

三項演算子を使用すると、コードをより簡潔にすることができます。

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

score 0 · Accepted Answer

私が行う方法は、一般的な Factory メソッドを持たないことです。

コマンドオブジェクトとしてドメインオブジェクトを使用するのが好きです。私は Spring MVC を使用しているので、DataBinder.setAllowedFields メソッドを使用すると、単一のドメインオブジェクトを複数の異なるフォームに使用する柔軟性が大幅に向上するため、これは優れたアプローチです。

コマンドオブジェクトを取得するために、Domain オブジェクトクラスに静的ファクトリメソッドを用意しました。たとえば、メンバークラスには次のようなメソッドがあります-

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

等々。

これが素晴らしいアプローチかどうかはわかりません。どこにも提案されているのを見たことがなく、自分で思いついたようなものでした. 異議を唱える理由がある場合は、コメント欄でお知らせください...

score 0 · Accepted Answer

直感に従って、反省してください。ただし、このソリューションでは、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);
     }
}

score 0 · Accepted Answer

うーん、ブラウジングして、これに出くわしただけです。まだコメントできますか？

IMHO元の if/else ブロックコードに問題はありません。これは単純であり、単純さは常に設計の最初の呼び出しでなければなりません ( http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossivelyWork )

提供されるすべてのソリューションは、元のコードよりも自己文書化がはるかに少ないため、これは特に真実のようです...つまり、翻訳ではなく読み取り用にコードを書くべきではありません...

score 0 · Accepted Answer

この反復的なオブジェクト作成コードをすべてファクトリに隠しておくことは、それほど悪いことではありません。どこかでやらなければならないことがあれば、少なくともここにすべてあるので、あまり心配する必要はありません。

本当にそれについて何かしたい場合は、Map を使用するかもしれませんが、プロパティファイルから構成し、その props ファイルからマップを構築します。

クラスパスの発見ルート (私にはわかりません) に行かなければ、常に 2 つの場所を変更することになります: クラスを作成し、どこかにマッピングを追加します (ファクトリ、マップ初期化、またはプロパティファイル)。

score 0 · Accepted Answer

これについて考えると、次のような小さなインスタンス化クラスを作成できます。

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 の結果を返します。もちろん、最初の 8 文字を CommandCreator の外部で抽出するのは少し不自然に見えるので、コマンド文字列全体を CommandCreator に渡すように作り直します。

誰かがそれについて不思議に思っている場合に備えて、ここで「スイッチをポリモーフィズムに置き換える」-リファクタリングを適用したと思います。

score 0 · Accepted Answer

リフレクションを介してマップと作成を行います。クラスパスのスキャンが遅すぎる場合は、いつでもカスタムアノテーションをクラスに追加し、コンパイル時にアノテーションプロセッサを実行して、すべてのクラス名を jar メタデータに格納できます。

そうすれば、できる唯一の間違いは、注釈を忘れることです。

maven とAPTを使用して、このようなことを少し前に行いました。

score -1 · Accepted Answer

少なくとも、コマンドには getCommandString() が必要です。ここで、StartCommand がオーバーライドされて「START」が返されます。その後、クラスを登録または検出するだけです。

score -1 · Accepted Answer

リフレクションの提案に+1すると、クラスでより健全な構造が得られます。

実際には、次のことを行うことができます (まだ考えていない場合) getCommand() ファクトリメソッドへの引数として期待する String に対応するメソッドを作成し、次に行う必要があるのは、reflect と invoke( ) これらのメソッドを実行し、正しいオブジェクトを返します。

java - Javaファクトリーメソッドのリファクタリング

18 に答える 18

Related

Reference