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)

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

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

stack overflow だね

２つ目は、デバッグしにくいことです。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にはデメリットが、２つあります。

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

２つ目は、ひとまとまりの処理の途中に、無関係なコードを記述できてしまうこと（下記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の２つのデメリットは、メソッド化することで対処できます。

現在のクラスでメソッド化してもいいし、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章 巨大な式を分割する