リーダブルコードcakePHP版?find()の引数並びにarray()を記述しない

cakePHPの公式サイトには次のようなサンプルコードがある。引数並びにarray()を記述している。

$allPublishedAuthors = $this->Article->find('list', array(
	'fields' => array('User.id', 'User.name'),
	'conditions' => array('Article.status !=' => 'pending'),
	'recursive' => 0
));

この書き方のメリットは、下記の書き方と比較するとわかりやすい。array()を$paramsなどの変数に代入していないので前後の変数との関連を考えなくてもいいこと(下記 A,C)、この1まとまりの処理の途中に無関係なコードを記述できないこと(下記B)だと思う。

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

しかし実務では次のデメリットのほうが大きいと思う。

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

2つ目はデバッグしにくいこと。printデバッグしたくても、引数並びのarray()のままではvar_dump()できない。eclipse+xDebugでトレースする場合も、引数並びのarray()を表示することはできない。ソースファイルがあれば関数へステップインすると表示できるが、組み込み関数の場合はステップインもできない。

次のようにしたほうが読みやすくデバッグもしやすいと思う。

$params = array(
	'fields' => array('User.id', 'User.name'),
	'conditions' => array('Article.status !=' => 'pending'),
	'recursive' => 0
);
$allPublishedAuthors = $this->Article->find('list', $params);

さらに、array要素を1行1要素にすると、gitで直前のリビジョンとdiffしたとき変更箇所がわかりやすい。

$params = array(
	'fields' => array(
		'User.id',
		'User.name',
 	),
 	'conditions' => array(
 		'Article.status !=' => 'pending',
 	),
 	'recursive' => 0,
);
$allPublishedAuthors = $this->Article->find('list', $params);

アクション内で$this->XXX->find()を何回も呼んでいて行数が多いと感じたら、それぞれのfind()呼び出しをモデルのメソッドにするといいと思う。モデルのメソッドにすれば1行で呼び出すことができ、前後の変数との関連はないし、一連の処理の途中に無関係なコードの記述もできない。

$allPublishedAuthors = $this->Article->find_list_ArticleStatusIsPending();

なお、viewの$this->input()の2つ目の引数は、ロジックとは関係がないhtmlタグやcssなどが多く、引数並びにarray()を記述したほうが読みやすいと思う。

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