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

https://stackoverflow.com/questions/115269

java
factory

02-07-2019
|

質問

このコードには非常に不満な点があります。

/*
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 のサブクラスのクラスパスをスキャンし、文字列表現をクエリして、それらをレパートリーに自動的に追加できればさらに良いでしょう。

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

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

解決

文字列とコマンドのマップは良いと思います。文字列コマンド名をコンストラクターに因数分解することもできます (つまり、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();
    }
}

enum の 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 を使用します。その後、使用できます CommandFactory.START.create(String cmd)コマンドオブジェクトを作成します。

最後の利点は、Javadoc ドキュメントで使用可能なすべてのコマンドのリストを簡単に作成できることです。

例外として、

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

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

おそらく最初の 8 文字だけが必要な理由を文書化するか、その文字列のどの部分がコマンドであるかを簡単に理解できるようにプロトコルを変更する必要があります (例:'：'または ';'のようなマーカーを置くコマンドキーワードの後）。

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

そうできない理由がない限り、私は常にコマンドの実装をステートレスにするように努めています。その場合は、このインスタンスが指定された文字列識別子に使用できるかどうかを示すメソッド 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);
    }
}

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

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

コマンドが必要なときは、コマンドを複製するだけです。

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

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

command.setCommandString(cmdName);

しかし、 clone() を使用するのは、リフレクションを使用するほどエレガントではありません:(

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

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

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

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

1) 一般的なファクトリーでは new とは呼びません。そのため、将来的には、各コマンドタイプが、文字列内のスペースパディングに続く引数に応じて、異なるクラスのオブジェクトを返す可能性があります。

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

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

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

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

クラスパス検出ルート (それについてはわかりません) をたどらないと、常に 2 か所を変更することになります。クラスを作成し、マッピングをどこか (ファクトリー、マップ初期化、またはプロパティー・ファイル) に追加します。

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

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 に渡すように修正します。

疑問に思っている人のために、ここでは「スイッチをポリモーフィズムで置き換える」リファクタリングを適用したと思います。

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

この場合、唯一の間違いは注釈を忘れることです。

私は少し前に Maven を使用してこのようなことを行いました。 APT.

私のやり方は、汎用の Factory メソッドを持たないことです。

私はドメインオブジェクトをコマンドオブジェクトとして使用するのが好きです。私は Spring MVC を使用しているので、これは素晴らしいアプローチです。 DataBinder.setAllowedFields メソッドこれにより、単一のドメインオブジェクトを複数の異なるフォームに使用する非常に柔軟な対応が可能になります。

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

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

等々。

これが素晴らしいアプローチかどうかはわかりません。これが提案されているのをどこにも見たことがなく、自分で思いついただけです。このようなものを 1 か所にまとめておくというアイデアが気に入っています。反対する理由がある場合は、コメントでお知らせください...

可能であれば反射を避けることをお勧めします。なんだか邪悪ですね。

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

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

enum を導入できます。各列挙型を定数ファクトリにするのは冗長であり、実行時メモリのコストもかかります。ただし、列挙型を簡単に検索して、それを == またはスイッチとともに使用できます。

 import xx.example.Command.*;

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

腹を立てて反省してください。ただし、このソリューションでは、newInstance を簡単に使用できるように、Command インターフェイスに setCommandString(String s) メソッドがアクセス可能であると想定されます。また、commandMap は、対応する Command クラスインスタンスへの文字列キー (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);
     }
}

うーん、ブラウジングしていて、たった今これを見つけました。まだコメントしてもいいですか？

私見ですが、あります 何もない 元の if/else ブロックコードが間違っています。これはシンプルであり、デザインにおいては常にシンプルさが最初に求められるものでなければなりません (http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork)

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

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

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

実際、次のことを行うことができます（まだ考えていない場合は）getCommand（）ファクトリーメソッドへの引数として期待する文字列に対応するメソッドを作成することができます。）これらの方法と正しいオブジェクトを返します。

ライセンス： CC-BY-SA と帰属

所属していません StackOverflow

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

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