勝手に添削: PHP初心者向けのコード最適化

PHP初心者向けのコード最適化という記事がちょっと社内で話題になったので勝手に添削します。話題になった理由は、確かにコードは最適化できてるけど圧倒的に汚くなってるからです。

この記事を読む前に、まずリーダビリティについて意識合わせしておきましょう。リーダビリティとは、他のプロジェクトメンバーが「そのコードが何をしたいかを把握する時間＋そのコードをレビューして正しいと自信を持てるまでの時間」の短さと定義します。

たとえば、コメントは「コメントとコードの一貫性をチェックする時間」が増えるので、「何をしたいかを把握する時間」を大幅に短くしない場合はむしろリーダビリティを下げます。

まずは最適化後のコードを引用します。コメントがちょっとクドかったので省略しています。

<?php
class TestAction
{
    public function register()
    {
        $testId = $this->getPostRequestParam('testId', 'Test');
        if (!$this->isValidId($testId)) {
            $this->redirect(array('controller' => 'test', 'action' => 'index'));
        }
     
        $sampleList = $this->Sample->getList($testId);
        if (empty($sampleList)) {
            return $this->render('error');
        }
     
        while (list(, $data) = each($sampleList)) {
            $dataNameIdList[] = $data['Sample']['name_id'];
        }
        reset($sampleList);
     
        $nameList = $this->Name->getList($dataNameIdList);
        while (list(, $data) = each($sampleList)) {
            while (list(, $nameData) = each($nameList)) {
                if ($nameData['Name']['name_id'] == $data['Sample']['name_id']) {
                    $data['Sample']['name'] = $nameData['Name']['name'];
                    $data['Sample']['category'] = self::CATEGORY_TEST; 
                    $this->Sample->save($data);
                    break;
                }
            }
            reset($nameList);
        }
     
        return $this->render('complete');
    }
     
    private function isValidId($id) {
        if (!is_int($id) || $id < 0) {
            return false;
        }
        return true;
    }
}

このコードがダメな理由は3つあります。

foreach を使わずに内部イテレータを使っている
2重ループはループ2つに分割でき、そのほうがパフォーマンスもリーダビリティもいい
というかモデルをアクションに書くな

1 は、 php は Python ほど特定のスタイルが強く推奨されているわけではないですが、それでも内部イテレータより foreach を使うというのはさすがに合意が取れると思います。内部イテレータは構文と完全に独立したスコープをもった暗黙の状態を使っているからです。たとえば、ループ内で呼び出した他の関数の中で reset() されてるせいで無限ループとかあほらしいバグを生みます。

2 は下記の改善例を見てください。3はあくまでもサンプルコードだからということで無視します。

では、改善例です。私は Cake を知らないで推測で書いているのですが、 name の array の先頭が ['Name'] だったり、 sample の先頭が ['Sample'] だったりするのはクドかったので省略しました。いろいろ間違ってるかもしれませんがそれはごめんなさい。

1 と 2 を改善することで、コードが簡潔になっています。簡潔さは「何をしたいかを把握する時間」と「レビューする時間」の両方に大きく影響します。

言語とリーダビリティに続く