読者です 読者をやめる 読者になる 読者になる

methaneのブログ

このブログに乗せているサンプルコードはすべてNYSLです。

勝手に添削: 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つあります。

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

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

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

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

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

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