Webで見かけた良くない例
の
には、ファイルのコピーをするCFileCopyというクラスを例に、メンバー関数が多すぎるクラスを手直しする例が紹介されている。
だが、この例を見ると、リファクタリングによって、不適切なインターフェースを持つクラスができてしまっている。
まずは、手直しする前の、メンバー関数が多過ぎるCFileCopyクラスを引用する。
class CFileCopy {
typedef bool (*UserCancelFunc)(const CFileCopy&,void*);
typedef void (*DirDownFunc)(const CFileCopy&,const char*,void*);
typedef void (*DirUpFunc)(const CFileCopy&,const char*,void*);
typedef void (*FileCopyStartFunc)(const CFileCopy&,const char*,void*);
typedef void (*FileCopyEndFunc)(const CFileCopy&,const char*,void*);
...(中略)...
public:
CFileCopy();
virtual ~CFileCopy();
void SetSourceDirName(const char*); //コピー元のディレクトリ名をセットする
void SetDestDirName(const char*); //コピー先のディレクトリ名をセットする
void SetCreateDir(bool); //コピー先のディレクトリが存在しないときに作成するかの指定
void SetUserCancelFunc(UserCancelFunc); //コピー中のユーザキャンセルの入力受け付けコールバック関数のセット
void SetUCFuncMisc(void*); //ユーザキャンセルの入力受け付けコールバックの第2引き数のセット
void SetDirDownFunc(DirDownFunc); //ディレクトリ降下時に呼び出すコールバック関数のセット
void SetDDFuncMisc(void*); //ディレクトリ降下時のコールバックの第3引き数のセット
void SetDirUpFunc(DirUpFunc); //ディレクトリ上昇時に呼び出す
//コールバック関数のセット
void SetDUFuncMisc(void*); //ディレクトリ上昇時のコールバックの第3引き数のセット
void SetFileCopyStartFunc(FileCopyStartFunc); //１つのファイルコピー開始時に呼び出すコールバック関数のセット
void SetFCStartFuncMisc(void*); //１つのファイルコピー開始時のコールバックの第3引き数のセット
void SetFileCopyEndFunc(FileCopyEndFunc); //１つのファイルコピー終了時に呼び出すコールバック関数のセット
void SetFCEndFuncMisc(void*); //１つのファイルコピー終了時のコールバックの第3引き数のセット
void ExecCopy(void); //コピーする
int GetErrorCode(void); //エラーコードをえる
};
次に、著者である真紀俊男氏が、このクラスを手直しした結果を引用する。
class CFileCopy {
class CallBack {
protected:
void* mMisc;
public:
CallBack(){ mMisc = NULL; };
virtual ~CallBack();
virtual bool UserCancel(void*) = 0; //コピー中のユーザキャンセルの入力受け付け
virtual void DirDown(const char*,void*) = 0; //ディレクトリ降下
virtual void DirUp(const char*,void*) = 0; //ディレクトリ上昇
virtual void FileCopyStart(const char*,void*) = 0; //１つのファイルコピー開始
virtual void FileCopyEnd(const char*,void*) = 0; //１つのファイルコピー終了
void SetMisc(void* ioMisc){ mMisc = ioMisc; }; //Misc引き数のセット
};
public:
CFileCopy();
virtual ~CFileCopy();
int SetParam(const char*,const char*,bool,CallBack*); //パラメータをセットする
int ExecCopy(void); //コピーする
};
class MyFileCopyCallBack : public CFileCopy::CallBack {
public:
virtual bool UserCancel(void*);
virtual void DirDown(const char*,void*);
virtual void DirUp(const char*,void*);
virtual void FileCopyStart(const char*,void*);
virtual void FileCopyEnd(const char*,void*);
};
この例では、CFileCopyクラスのメンバー関数の数を減らそうとした結果、次のようなSetParamという関数を作ってしまった。
int SetParam(const char*,const char*,bool,CallBack*); //パラメータをセットする
コーダーの視点が、不適切なクラスを作り出す
著者である真紀俊男氏は、手直しする前のクラスの問題点を、次のように指摘している。
コールバック関数の設定がやたら多く，コピーそのものを実行するまでの前準備が大変です。また，どれが必須の呼び出しなのか，どの順番で呼ばないといけないのか，調べることがたくさん出てきそうです。
そして、次のようなリファクタリングの方針を掲げている。
もしも筆者が，これと同じようなクラスを作るとすれば，List 2のようにコールバック関数に関してはコールバック専用のクラスを作って，そちらに機能委譲をさせ，またパラメータ設定は，まとめて１つのメンバ関数にしてしまい(こうすることで呼び出し忘れ，呼び出し順番をどうするかの悩みが解決します)，すっきりする方向にまとめるでしょう。
これは、このCFileCopyクラスを利用したプログラムをコーディングする、コーダーの視点から見た意見であろう。
元々、この記事は、雑誌「C MAGAZINE」に掲載されたものであるため、C言語プログラマーの視点で書かれているように思われる。
だが、コーダーの視点に基づいてクラスを作り直すと、逆に、クラスが分かりにくく、使いづらくなってしまうこともある。
この例では、確かに、CFileCopyクラスのメンバー関数の数は劇的に減少した。コピーをするまでの前準備は１行で済むし、関数を呼び出す順序を間違えることも無くなっただろう。
しかし、パラメータの設定をSetParam関数でまとめたことにより、却って、CFileCopyクラスの機能が分からなくなってしまった。手直しした後のクラスでは、コピー先のディレクトリが存在しないときに作成するかの指定が行えることを、インターフェースから読み取ることはできない。
また、本来は互いに無関係な、独立したパラメータを、すべて一度に指定するようにしたため、クラスの使い方に制限が生じてしまった。例えば、あるディレクトリを２ヵ所にコピーする場合は、コピー先のディレクトリ名だけを変更してからExecCopy関数を呼びたいところだが、手直ししたクラスでは、すべてのパラメータを設定し直さなくてはならない。
デザイナーの視点で、クラスを作り直す
メンバー関数を少なくすれば、分かりやすいクラスになる訳ではない。コーディングが１行で済むのが、使いやすいクラスとは限らない。
例えば、Java言語のFileクラスは、非常に良く使われている基本的なクラスである。このクラスは、実はかなりメンバー関数が多い。だが、これを分かりにくく使いにくいと思っている人は少ないはずだ。
リファクタリングを行い、クラスを作り直すのであれば、コーディングをする人の視点ではなく、クラスを設計するデザイナーの視点で考えることが重要だ。
デザイナーの視点で、クラスの役割や、あるべき姿を考えると、ファイルをコピーするCFileCopyクラスのインターフェースは、次のようにした方が良いだろう。
- パラメータは個別に指定する。
-
プログラミングの禁じ手Web版 C++編で紹介されている例では、下記の4つのパラメータを一度に設定するSetParam関数が作られた。
- コピー元のディレクトリ名
- コピー先のディレクトリ名
- コピー先のディレクトリが存在しないときに作成するかの指定
- 処理が行われた時に呼び出されるコールバック専用のクラス
だが、これら4つのパラメータは、必ず同時に指定しなければならないものではない。むしろ、互いに独立なものである。
そこで、これらを別個に指定できるように、4つのメンバー関数に分ける。この点に関しては、事実上、リファクタリングをする前の方が良かった、と言える。
- コピー元のディレクトリ名、コピー先のディレクトリ名は、コンストラクタで指定する。
-
CFileCopyオブジェクトのあるべき姿を考えると、この２つのパラメータは必須なので、コンストラクタで指定する。
プログラミングの禁じ手Web版 C++編で紹介されている例では、空のコンストラクタしか用意していないが、生成直後のオブジェクトの立場が考えられていない。絶対にSetParam関数を呼ばなくてはいけない、という、暗黙の制約がある。
- 通知先は複数登録できるようにする。
-
プログラミングの禁じ手Web版 C++編で紹介されている例では、通知をするCallBackの相手を１つしか指定できない、という制限があった。だが、CFileCopyクラスのあるべき姿を考えれば、通知先は１つだけとは限定されないはずである。
自分がそのクラスを使う時の、目先の使い方のことしか頭に無いと、このような不要な制限を入れてしまいやすい。
- フラグの指定を増やす。
-
プログラミングの禁じ手Web版 C++編で紹介されている例では、コピー先のディレクトリが存在しないときに作成するかの指定しかできないようになっていた。
だが、CFileCopyクラスのあるべき姿を考えると、次のような指定もあった方が良い。
- コピー先のファイルがすでに存在した場合に、上書きするか。
- ある１つのファイルでエラーが起きた時に、その時点で処理を中止するか、続行するか。
ちなみに、プログラミングの禁じ手Web版 C++編で紹介されている例では、このような要望があった時、SetParam関数の引数をどんどん増やすハメになってしまう、という問題もある。
ちなみに、プログラミングの禁じ手Web版 C++編で紹介されている例では、GetErrorCodeという関数が削除され、ExecCopy関数がエラーコードを返すように直されていた。これはおそらく、ExecCopy関数とGetErrorCode関数と、2つの関数を続けて呼び出すのが面倒だから、というコーディング上の理由で直されたように思われる。
だがこれも、単に呼び出すのが面倒だから、という理由ではなく、CFileCopyクラスの役割から判断するべきだ。CFileCopyクラスは、実行状態や実行結果を表すものではないので、エラーコードは持つべきではないのである。
Webで見かけた例の、その他の問題点
プログラミングの禁じ手Web版で紹介されているリファクタリングの例では、その他にも、いくつかの問題点がある。最後に、それらの問題点を指摘しておく。
- ヘッダーファイルに、引数の変数名が書かれていない。
-
引数に何を渡せば良いのかを、引数の変数名で表すべきである。
- コメントが意味を為していない。
-
例えば、SetParam関数のコメントは「パラメータをセットする」となっているが、これは何も書いていないのと同じである。
- voidポインタが使われている。
-
これでは、引数としてどのようなデータが渡されるのかが分からない。つまり、クラス間のインターフェースを定義していないのと同じことになっている。
キャストを使え、ということなのだろうが、C++言語でのキャストは危険が場合が多い。また、正しくキャストするためには、クラス間で暗黙の了解が必要になる。
- 名前が不適切。
-
CallBackという、C言語プログラマーらしい名前が付けられているが、ここはデザインパターンのObserverパターンが使われているので、CFileCopyObserverなどの名前が良い。
また、通知をする関数の名前が、「DirDown」「FileCopyStart」のようになっているが、これでは処理を実行するように思えるので、通知する関数であることを分かるような名前にする。例えば、「NotifyDirDown」や、「FileCopyStarted」など。