【しばらく編集不可モードで運営します】 編集(管理者用) | 差分 | 新規作成 | 一覧 | RSS | FrontPage | 検索 | 更新履歴

WritingTestableCodeFlaw1ConstructorDoesRealWork - テストできるコードの書きかた::まずいのその1: コンストラクタがやりすぎ

目次

テストできるコードの書きかた::まずいのその1: コンストラクタがやりすぎ

この文書について

Misko Hevery

コンストラクタの中で仕事をやってしまうと、テストするのに必要なシーム (訳注: WorkingEffectivelyEithLegacyCode? でも出てきます。あわせて読みたい) がなくなってしまうし、サブクラス/モックに継承したくない振る舞いまで継承させてしまうことになる。 ここで言っている仕事というのは以下のようなもの: 関係するオブジェクトを生成/初期化したりする、他のサービスと通信する、自身の状態を構築するためのロジック、など。 コンストラクタでやりすぎると、テストするときに関係するオブジェクトをつくったり変更するのがむずかしくなってしまうよ。

あぶない予兆

なにがまずいの?

コンストラクタで、関係するオブジェクトを生成したり初期化してしまうと、 柔軟性に欠けるし、早まって設計を密結合にしてしまうことになる。 テストするときには、関係するオブジェクトはそれ用のものを注入したいものなんだけど、こうしたコンストラクタはそのチャンスを殺してしまう。

単一責任原則 (SRP) に違反

初期化処理の中に関係するオブジェクトの生成を混ぜこむということは、オブジェクトの初期化の方法が一つしかないということを暗に示している。 これは、再利用できるはずだったクラスの再利用の可能性を閉ざしているということだ。 オブジェクト間の接続関係の構築は、それだけで立派な一つの責務である ― そのクラスがそこに存在する理由とはまた別の責務だ。 コンストラクタでそのような処理を行うということは、単一責任原則に反していることになる。

直にテストするのがむずかしい

上記のようなコンストラクタはテストするのが難しい。 オブジェクトを生成する際には、コンストラクタは必ず実行される。 もしコンストラクタがたくさんの処理を行っているとすると、テストのときにオブジェクトを生成する際にも、 コンストラクタの処理に合わせていろいろ準備する必要が出てきてしまう。 関係するオブジェクトが外部のリソース(ファイル、ネットワークサービス、データベースなど)にアクセスしている場合は、 関係するオブジェクトのちょっとの変更がコンストラクタに影響してしまうけれど、 それもテストケースが不足していたために見逃されてしまう可能性がある。 テストケースが書かれていないのはコンストラクタのテストの難しさが原因だ。 といった具合に、最終的には厄介なスパイラルにはまり込んでしまう。

テストをサブクラス化したり、オーバーライドすると、これまたまずい

他にも、コンストラクタそれ自身はちょっとしか処理をしていないように見えるけれど、 実際には別のメソッドへ処理を委譲しているケースというのがある (このメソッドはテスト用のサブクラスでオーバーライドされることを期待している)。 これはオブジェクトの生成が面倒だという問題の解決にはなるけれど、 実際この「テスト用のサブクラス」を作るというトリックは、最後の手段としてのみ用いるべきだ。 さらに、サブクラス化をするとなると、そこでオーバーライドしたメソッドのテストはどうする?という問題が出てくる。 そのメソッドではいろいろ処理をしているわけだから (思い出してみよう ― そもそもこれがこのメソッドを作った理由だった)、 つまり、テストしないとまずい。

関係するオブジェクトを束縛してしまう

オブジェクトのテストを行っていると、関係するオブジェクトのうちいくつかは生成したくない、というケースが発生することがある。 例えば、MySql?とやり取りを行うMySqlRepository?というオブジェクトがあって、こいつは生成したくないとしよう。 もしMySqlRepositoryServiceThatTalksToOtherServers?()メソッドを使ってこいつを直接生成していたとすると、 テスト環境(System Under Test (SUT))で重たいオブジェクトを使うのを強制されてしまうことになる。

シームが消えてしまう

シーム(縫い目)というのはコードベース上の境目のことだ。 シームでコードベースを区切ると、他と依存関係のないオブジェクトを切り出すことができる。 また、シームからは、小さくて特定の責務だけにフォーカスしたオブジェクトを生成することができる。 例えば、コンストラクタの中に new XYZ() という処理があると、 もうここで別のオブジェクト(サブクラスのオブジェクト)を使わせることは不可能になってしまう。 (シームについてより詳細に知りたい場合は、Michael Feathersの「Working Effectively With Legacy Code」を参照するとよい。)

コンストラクタがいくつかあっても (そのうちのいくつかはテスト専用)、まだまずい

「テスト専用」のコンストラクタを作るという方法もあるが、解決になっていない。 いろいろ処理をしている(訳注:テスト用でない)コンストラクタが他のクラスから呼ばれているという状況に変わりはない。 このオブジェクトだけを個別に(テスト専用コンストラクタを生成に使って)テストしたとしても、 最終的にはテストの難しいコンストラクタを呼んでいるクラスにぶち当たる。 その段になったら、もう身動きは取れない。

最低ライン

最終的には、クラスを個別に生成したり、関連するクラスをテスト用ものに置き換えたりするのが簡単か困難かという点に帰着する。

コードを書くときは、そのオブジェクトをテストするのがどのくらい難しいかを常に考えておくべきだ。 あなたの書いたコンストラクタでオブジェクトを生成するのは簡単ですか? (そのオブジェクトの生成が行われる箇所のは、自分で書いたテストクラスの他にもあるということは心に留めておいたほうがいい。)

"オブジェクトが他のオブジェクトを生成したり、オブジェクトをグローバルな領域から取ってきたりするような処理"でいっぱいの設計は数多くある。 "このようなプログラミングの習慣を放っておくと、テストするのが難しい、密結合な設計を生み出してしまう"。 [J.B. Rainsberger, JUnit Recipes, Recipe 2.11]

理解してみよう

以下のような兆候がないかチェックしてみよう:

コードを書いたりレビューしたりするときには、次の基本的な質問を考えてみよう:

どうやってこいつをテストしようか?

"答えが明確でない場合や、テストが醜くなったり書くのが難しいものになりそうだったら、それを警告のシグナルだと考えてください。 おそらく設計を変更する必要があるでしょう;簡単にテストができるように変更を行えば、その労力に見合った設計になるはずです。" [Hunt, Thomas. Pragmatic Unit Testing in Java with JUnit, p 103 (ちょっと古いですが、いい本で手軽に読める)]

注意:値オブジェクトを作成するだけであれば、多くの場合問題はない (例:LinkedList?; HashMap?, User, EmailAddress?, CreditCard?など)。 値オブジェクトのキーとなる特性としては以下の3点がある: (1) 簡単に作成できる (2) 状態を表現することに焦点を置いている(getter/setterが多く、振る舞いを表現するメソッドがあまりない) (3) サービスオブジェクトを参照していない

直してみよう

関連するオブジェクトをコンストラクタで作らずに、引数で渡すようにしよう

オブジェクトグラフの構築と初期化に関する責任は、他のオブジェクトに移動してしまおう(例:builder, factory, providerなどのオブジェクトは取り除いて、それらのオブジェクトをコンストラクタに引数として渡すようにしよう)。 例:クラスがDatabaseService?(これはインタフェースであることを期待しよう)に依存している場合、依存性注入(DI)を使って、必要なDatabaseService?のサブクラスのオブジェクトをコンストラクタに渡すようにしよう。

繰り返す:関連するオブジェクトはコンストラクタで作らずに、引数で渡すようにしよう。(Don’t look for things! Ask for things!) 初期化処理で使うオブジェクトを渡す必要がある場合、選択肢は3つある:

1. Guiceを使っている場合のベストアプローチ:Provider<YourObject?>を使って、コンストラクタの引数に渡すオブジェクトを初期化しよう。オブジェクトの初期化とオブジェクトグラフの構築の責務はGuiceの方に移してしまおう。これで、あちこちにオブジェクトの初期化を書く必要がなくなる。Provider以外にも、BuilderやFactoryを使いたい場合もあるだろうが、そういう場合もBuilderやFactoryをコンストラクタに渡すようにしよう。

2. 手動で依存性注入を行っている場合のベストアプローチ:YourObject?のコンストラクタの引数にBuilder, Factoryなどのオブジェクトを渡すようにしよう。通常はオブジェクトグラフ全体を作るのにFactoryが一つだけあればいい。下の例を参照すること。(このため、各クラスごとにファクトリを作ってクラス数爆発が起きてしまうということはないので、心配しなくてよい)Factoryの責務は、オブジェクトグラフを作成し、そしてそれ以外の処理は何もしないことである。(ファクトリの中身を見ると、大量のnewキーワードと、リファレンスを渡す処理が書かれているだけのようになっているはず。)オブジェクトグラフの責務は、処理を行い、オブジェクトの生成は行わないことである。(アプリケーションロジックを記述したクラスの中では極端にnewキーワードが少なくなっているはず。)

3. 最終手段:クラスにinit(…)メソッドを作って、オブジェクトを生成した後でも呼べるようにする。できる限りこの方法は避け、パラメタの初期化・設定だけを責務として持つ別のオブジェクトを作るようにしよう。(Guiceを使っているなら、これはProviderになったりする)

(以下に示すコード例も読もう)

実際のコードによる劇的ビフォアアフター

基本的に、"コンストラクタで何かやってる"というのは、そのオブジェクトの初期化を難しくしているとか、テスト用のオブジェクトを使うのを難しくしているというのと同じことだ。

問題: new キーワードがコンストラクタの中にあるか、メンバ変数の宣言にある

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-new-Keyword-in-the-Constru を参照)

この例ではオブジェクトグラフの作成とロジックとを混ぜこぜにしてしまっている。テストの際には、本番環境とは異なるオブジェクトグラフを生成したいという場合がよくある。そのような場合は通常、本番環境より小さく、一部のオブジェクトがテスト用のクラスで置き換えられたグラフを生成したいと思うことだろう。newオペレータをインラインで書いておくと、そのようなテスト用のオブジェクトグラフを作ることができなくなってしまう。"newオペレータについての考え方"(http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/)も参照のこと。

Kitchenが値オブジェクト(Linked List, Map, User, Email Address等)であって、それらのオブジェクトがサービスオブジェクトへの参照を持っていないのであれば、インラインで生成を行ってもかまわない。サービスオブジェクトはテスト用のオブジェクトで置き換える必要がある典型的な例だから、それを直接生成したりstaticメソッドを呼んで生成したりしようなどと思ってはいけない。

問題: 完全に初期化されてないオブジェクトを引数にとり、初期化の続きをしないといけない

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Constructor-takes-a-partia を参照)

オブジェクトグラフの作成(Gardenオブジェクトと関連を持つオブジェクトの生成・設定)は、Gardenクラスが果たしている責務とはまた別の責務である。設定と生成がコンストラクタの処理に混ぜ込まれてしまうと、オブジェクトはより硬直化し、具象的なオブジェクトグラフの構造に密に結合されてしまう。これにより、コードは変更が難しく、(程度の多少はあれど)テスト不可能なものになってしまう。

Have two objects when you need to have collaborators initialized. それを初期化した後に、完全に初期化済みのオブジェクトを対象クラスのコンストラクタに渡すようにしよう。

問題: コンストラクタがデメテルの法則に違反している

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Violating-the-Law-of-Demet を参照)

この例では、アプリケーションのグローバルエリアからsingletonであるRPCClientを取得している。だが見て分かるように、本当に欲しいのはUserオブジェクトであって、何かsingletonなオブジェクトが欲しいわけではない。 First: we are doing work(staticメソッドを使っているので、シームはなくなっている)。第2に:このコードは「デメテルの法則」に違反している。

改善されたコードでは本当に必要なものだけがコンストラクタに渡されている:Userオブジェクトである。テストの際に作成しなければならないのは(本物であるにしろテスト用であるにしろ)Userオブジェクトだけだ。この変更により、設計はより柔軟になり、テストも容易になる。

ここではRPCClientからUserオブジェクトを取得するのにGuiceを使用している。単体テストではGuiceを使用せず、UserオブジェクトとAccountView?オブジェクトを直接生成する。

問題: 要らないサードパーティのオブジェクトをコンストラクタでつくっている

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Creating-Unneeded-Third-Pa を参照)

文字通り、Car(自動車)オブジェクトが自分のエンジンを作成するためにEngineFactory?(エンジン工場)を持ってくるというのは理不尽だ。車を作るときには出来合いのエンジンが与えられているべきであって、そのエンジンの作成方法を理解するべきだというのはちょっと違うだろう。乗っている車が走り出そうとするときに、その車を作った工場への参照がないと走りだせないという訳じゃない。同様に、オブジェクトの動作に直接必要とされているわけではないサードパーティ製のオブジェクトを、コンストラクタが使ってしまっている場合がある ― 本当に必要なのは、そのサードパーティ製のオブジェクトから作成されるオブジェクトだけなのだ。

このようなサードパーティ製のオブジェクトは取り除いて、コンストラクタの中で行っている処理を単純な変数への代入に置き換えてしまおう。コンストラクタでは設定済みの変数をオブジェクトのフィールドに代入しよう。コンストラクタの引数に渡すオブジェクトを作成する、別のオブジェクト(Factory, Builder, GuiceのProviderなど)を作成しよう。主体となるオブジェクトからオブジェクトグラフの生成という責務を取り除いてやれば、より柔軟でメンテナンスの楽な設計になる。

問題: コンストラクタの中で直接フラグを読んでいる

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Directly-Reading-Flag-Valu を参照)

引数のないシンプルなコンストラクタに見えるが、実際には多くの依存性を抱えている。このAPIはプログラマを騙している。簡単に生成できるように見えて、実際にはPingServer?の柔軟性は失われており、グローバルな状態に密結合されている。

最終的にPingServer?が必要としているのはソケットであってポート番号ではない。ポート番号を渡してしまうと、テストを本物のソケットとスレッドで行うことになってしまう。ソケットを渡すようになっていれば、テストの際にはテスト用のソケットのモックを作って、本物のソケットやスレッドは一切使わずにテストを行うことができる。明示的にソケットを渡すようにするだけでも、グローバルな状態への依存性を排除し、テストを非常にシンプルなものにできる。もちろん最終的に必要とされているSocketオブジェクトを渡してやる方がずっと良い。

問題: コンストラクタの中で直接フラグを読み、その値に基づいてオブジェクトをつくっている

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Directly-Reading-Flags-and を参照)

GuiceにはFlagBinder?というものがある(訳注:FlagBinder?の名前で検索してもこのページしか引っかからないんですが……)。これは、フラグの参照を取り除いてそれを依存性注入で置き換える ― しかも非常に低コストで ― というものだ。フラグは実行時にパラメタを変更するための手段としてそこらじゅうで使われているが、グローバルな状態を取得するためにフラグを直接読まなければならないという訳ではないだろう。

FlagBinder?を使って、クラスの中にあるフラグをGuiceのスコープに移して依存性注入ができるようにしてしまおう。(FlagBinder?クラスを書くことで、コマンドラインフラグを依存性注入可能なパラメタにどう結びつけるかを規定できる)

問題: コンストラクタのお仕事を初期化メソッドに移している

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Moving-the-Constructor-s-w を参照)

「お仕事」を初期化用のメソッドに移してしまうというのは解決になっていない。オブジェクトの持つ責務が一つだけになるように、オブジェクトを分割する必要がある。(責務のうち一つは、完全に初期化の完了したオブジェクトグラフを提供する、というものだ)

このFlawを解決するには、他の問題と同様に、JVMによっても強制されているグローバルな状態への依存を取り除き、依存性注入を使用する必要がある。

問題: いくつかのコンストラクタがあり、うち1つはテスト専用だ

(ソースコードは http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/#TOC-Problem:-Having-Multiple-Constructo を参照)

コンストラクタが何個かあり、そのうちいくつかはテスト専用だというのは、そのコードの中にテストの難しい部分がまだ残っているというヒントになる。左側のVideoPlaylistIndex?は簡単にテストできる(テスト用のVideoRepository?を渡せるから)。だが、引数なしのコンストラクタに依存しているオブジェクトはテストするのが難しいままだ。

PlaylistGenerator?がコンストラクタの中でVideoPlaylistIndex?への依存関係を直に構築するのではなく、PlaylistGenerator?のコンストラクタがVideoPlaylistIndex?オブジェクトを要求するのが理想的だ。依存するオブジェクトを要求する形にPlaylistGenerator?を修正してしまえば、VideoPlaylistIndex?の引数なしのコンストラクタを呼んでいる箇所はなくなるので、これは削除できる。普通は、コンストラクタがいくつも必要になることはない。

FAQ