こんにちは。小川です。
本日はPHPユーザ会主催の設計勉強会が開催されていたのですが、応募期間に間に合わなかったので、鬱憤を晴らすためにブログを書いてみました。
1週間ほど前からsymfonyの公式ブログで、コントローラ(symfonyだとactions)にロジックを詰め込んでいるようなコードを、ストーリー形式でリファクタリングしていくという記事が5回に渡って紹介されていました。
symfony | Web PHP Framework | Blog Category | Call the expert
こちらの「A refactoring story」というのがその記事になります。
僕が最初にMVCフレームワークを使ったときがそうだったのですが、MVCを理解していないとコントローラに全てのロジックを詰め込んでしまうようなコードを書いてしまいがちなのではないかと思います。皆さんはどうでしょうか。心当たりのあるかたはぜひこの機会に見直していきましょう。
この記事ではPart3から実際にリファクタリングに入っていきます。Part3のはじめに、こんな1文があります。
The Controller role is to get data from the Model and pass them to the View.
「コントローラの役割はモデルからデータを取得しビューへの橋渡しをすること」と述べられています。コントローラにはロジックを詰め込むべきではないのです。
それでは実際にリファクタリングしていく様子をみていきましょう。
今回は簡単なECサイトが題材として使われています。全体のコードやテーブル定義はPart1の記事に書かれています。
Call the expert: A refactoring story (Part 3/5)
<?php
// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
// get all available products
$criteria = new Criteria();
$criteria->add(ProductPeer::IS_IN_STOCK, true);
$criteria->addAscendingOrderByColumn(ProductPeer::PRICE);
$criteria->setLimit(10);
$this->products = ProductPeer::doSelectJoinCategory($criteria);
$this->getResponse()->setTitle('All products');
$this->getResponse()->addStylesheet('homepage.css');
return sfView::SUCCESS;
}
これは在庫のある商品を価格の昇順に10件取得して表示するというアクションです。まずはこのコードから直していくようです。
このコードはCriteriaを用いて条件を定義し検索するという処理をコントローラ内で全て行っていますが、こういったデータベースから取得するコードはモデルに書くべきです。
ということで以下のように書き直されました。
Call the expert: A refactoring story (Part 3/5)
<?php
// lib/model/ProductPeer.php
static public function getAvailableProducts()
{
$criteria = new Criteria();
$criteria->add(self::IS_IN_STOCK, true);
$criteria->addAscendingOrderByColumn(self::PRICE);
$criteria->setLimit(10);
return self::doSelectJoinCategory($criteria);
}
<?php
// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
$this->products = ProductPeer::getAvailableProducts();
$this->getResponse()->setTitle('All products');
$this->getResponse()->addStylesheet('homepage.css');
return sfView::SUCCESS;
}
どうでしょうか。上がモデル、下がコントローラになりますが、コントローラ側はだいぶすっきりしたのではないでしょうか。
例えば、同じ条件で商品をサイドバーに表示したい！という要望があったときどうでしょう。
その場合も上記のgetAvailableProductsメソッドを使えば同じ内容のデータがすぐに取得できて楽ですよね。
さて、getAvailableProductsメソッドをもう1度みてみましょう。Limit値が10固定になってしまっていますよね。このままでは、このメソッドを使ってる別の部分では5件だけ取得したいといった要望がある場合に対応できません。これでは汎用性にかけるということで、以下のように修正されました。
Call the expert: A refactoring story (Part 3/5)
<?php
// lib/model/ProductPeer.php
static public function getAvailableProducts($max = 10)
{
$criteria = new Criteria();
$criteria->add(self::IS_IN_STOCK, true);
$criteria->addAscendingOrderByColumn(self::PRICE);
$criteria->setLimit($max);
return self::doSelectJoinCategory($criteria);
}
第一引数にLimit値を指定できるようになりました。デフォルト値に今までとおり10が入っていますので、既存の呼び出し部分を変更する必要はありません。
他にも例えば、特定の箇所でだけ価格の降順にしたいといった要望があった場合を想定して、自分なりに実装に手を加えてみました。
自分なり実装その1
<?php
// lib/model/ProductPeer.php
static public function getAvailableProducts($max = 10, $desc = false)
{
$criteria = new Criteria();
$criteria->add(self::IS_IN_STOCK, true);
$orderer = ($desc) ? 'addDescendingOrderByColumn' : 'addAscendingOrderByColumn';
$criteria->$orderer(self::PRICE);
$criteria->setLimit($max);
return self::doSelectJoinCategory($criteria);
}
これも同様にオプショナルな引数をつけることで簡単に対処してみました。
ただ上記のように引数をどんどん追加していった場合、第2引数や第3引数だけ指定したい場合にそれより前にある引数も必ず指定しなければいけなくなるため、常に引数の並び順やデフォルト値を把握しておく必要がでてきますよね。
このような場合はオプショナルな引数を連想配列で定義できるようにしておくと、順番は関係なくなり必要なものだけ指定できるようになるためそのような心配はなくなります。
自分なり実装その2
<?php
// lib/model/ProductPeer.php
static public function getAvailableProducts($args = array())
{
$args = $args + array('limit' => 10, 'desc' => false);
$criteria = new Criteria();
$criteria->add(self::IS_IN_STOCK, true);
$orderer = ($args['desc']) ? 'addDescendingOrderByColumn' : 'addAscendingOrderByColumn';
$criteria->$orderer(self::PRICE);
$criteria->setLimit($args['limit']);
return self::doSelectJoinCategory($criteria);
}
ヘルパの中にはこうしたオプションを連想配列で取得する実装が多くみられます。
どちらの形で実装するかはケースバイケースだと思います。そもそも1つの実装でまとめるのがベストとは限りませんからね。
上記のケースでは、個人的にはその1の実装の方がわかりやすいかなと思います。（下だと冗長にみえますね。）
話を記事の方へ戻します。この後Part3の後半ではお気に入り、Part4では商品編集のコントローラ部分からロジックを切り出しを行い、最終のPart5ではコントローラからさらにコードを排除するということを行っていきます。
先ほどのexecuteIndexアクションも最終的に、
Call the expert: A refactoring story (Part 5/5)
<?php
// apps/frontend/modules/product/actions.class.php
public function executeIndex()
{
$this->products = ProductPeer::getAvailableProducts();
}
この1行だけになります。まさに最初にあげたとおりモデルからビューへデータを渡すという役割に専念していますよね。
さてさて、こうしてリファクタリングを行ってきましたが、具体的にどのようなメリットがあったのでしょうか？
既に1つはおわかりかもしれませんが、再利用可能(re-usable)なオブジェクトとメソッドができましたよね。
機能を抽出することで変更もしやすくなりますし、何よりコードが読みやすくなりました。
これも1つ大きなことなのですが、ユニットテストがやりやすくなります。
モデルにテストしている最中に別の人がコントローラに修正を入れたいといったときでも大丈夫ですね。
こうしてみるとたくさんのメリットがありますね。ぜひ「コードのあるべき場所」というものを意識してコーディングしていきましょう。
最後に1点。Part3の最後でメソッド名の重要性について触れられています。以下を見てみてください。
Call the expert: A refactoring story (Part 3/5)
<?php if ($sf_user->hasInFavorites($product)): ?>
このコード、ある商品がユーザーのお気に入りかどうかをチェックしているのではないかと、なんとなく連想できませんか？
このようにパッとみただけでなんとなくメソッドの内容がわかるととてもやりやすいですよね。
僕もメソッド名を決めるときはすごく悩むのですが、それだけメソッド名には価値があると思います。
特に意識することは、できる限り文章のように読めることです。上記のコードも「User/has in favorites/product」のように文章に近い感覚で読み取れますよね。
あと意識している点は粒度をあわせることです。例えばお気に入りが「購入済み」と「未購入」に分かれてたとします。そのうち購入済みのお気に入りに商品が含まれているかどうかを調べるメソッドがあったとして、その名前が上記と同じhasInFavoritesだったらどうでしょうか。メソッド名をみただけではそれが購入済みかどうか判断できないですよね。
この場合は例えばhasInPurchasedFavoritesのように、内容と同じレベルの名前をつけてあげることで混乱を防げるようになると思います。
紹介は以上になりますがいかがでしたでしょうか。ぜひ興味を持たれた方は本文の方も読んでみてください。
それではみなさま、プログラミングをエンジョイしましょう 🙂