cakePHP: find()の引数並びにarray()を記述するとデバッグしにくい

オリジナル

cakePHPの公式サイトに次のようなサンプルコードがあります。

気になるのは、引数並びにarray()を記述していることです。

// オリジナル $allPublishedAuthors = $this->Article->find('list', array( 'fields' => array('User.id', 'User.name'), 'conditions' => array('Article.status !=' => 'pending'), 'recursive' => 0 ));
Code language: PHP (php)

この書き方のデメリットは2つあります。

1つ目は、引数並びが長すぎです。長すぎて読む気力が萎えてしまいます。find()の閉じ括弧までが長すぎて、頭の中のスタックのプッシュ量が多い感じがします。

stack overflow だね

2つ目は、デバッグしにくいことです。printデバッグしたくても、引数並びのarray()のままではvar_dump()できません。

デバッガでトレースする場合も、引数並びのarray()を表示することはできません。関数へステップインすると表示できますが、ステップインのひと手間が増えてしまいます。

改善案1

次のように、配列を変数に設定して、変数を引数で渡したほうが読みやすく、デバッグしやすくなります。

// 改善案1 $params = array( 'fields' => array('User.id', 'User.name'), 'conditions' => array('Article.status !=' => 'pending'), 'recursive' => 0 ); $allPublishedAuthors = $this->Article->find('list', $params);
Code language: PHP (php)

ただし、オリジナルと比べると、改善案1にはデメリットが、2つあります。

1つ目は、前後のコードで$params変数を使っていると、前後のコードに影響してしまうことです。(下記A、C)

2つ目は、ひとまとまりの処理の途中に、無関係なコードを記述できてしまうこと(下記B)です。

//A. これ以前に$paramsに代入しているかもしれない $params = array( 'fields' => array('User.id', 'User.name'), 'conditions' => array('Article.status !=' => 'pending'), 'recursive' => 0 ); //B. ここに無関係なコードを記述できてしまう。 $allPublishedAuthors = $this->Article->find('all', $params); //C. これ以降に$paramsを参照しているところがあるかもしれない
Code language: PHP (php)

改善案2

改善案1の2つのデメリットは、メソッド化することで対処できます。

現在のクラスでメソッド化してもいいし、Articleクラスのメソッドにしてもいいと思います。

// 改善案2 public function findAllPublishedAuthors() { $params = array( 'fields' => array('User.id', 'User.name'), 'conditions' => array('Article.status !=' => 'pending'), 'recursive' => 0 ); $allPublishedAuthors = $this->Article->find('list', $params); return $allPublishedAuthors; }
Code language: PHP (php)
$allPublishedAuthors = $this->findAllPublishedAuthors();
Code language: PHP (php)

これぐらいの小さい単位でメソッド化したほうがいいんだね

改善案3

さらに、array要素を1行1要素にすると、gitの変更履歴がわかりやすくなります。

// 改善案3 public function findAllPublishedAuthors() { $params = array( 'fields' => array( 'User.id', 'User.name', ), 'conditions' => array( 'Article.status !=' => 'pending', ), 'recursive' => 0, ); $allPublishedAuthors = $this->Article->find('list', $params); return $allPublishedAuthors; }
Code language: PHP (php)

さらに、$params生成をメソッド化してもいいと思います。

// 改善案3-2 public function findAllPublishedAuthors() { $params = $this->makeParamsTofindAllPublishedAuthor(); $allPublishedAuthors = $this->Article->find('list', $params); return $allPublishedAuthors; } public function makeParamsTofindAllPublishedAuthors() { $params = array( 'fields' => array( 'User.id', 'User.name', ), 'conditions' => array( 'Article.status !=' => 'pending', ), 'recursive' => 0, ); return $params; }
Code language: PHP (php)

関連:リーダブルコード 8章 巨大な式を分割する

タイトルとURLをコピーしました