PHPコードのレビュー結果を共有してみる

PHP Advent Calendar 2014 の9日目として書いています。

昨日は PHP CS Fixerで快適PHPライフ で、コーディング規約に沿うよう修正してくれるツールのお話でしたね。

今日は、最近、PHP初心者が書いたソースコードをレビューする機会があったのですが、その結果をいくつか共有してみようと思います。

1. range() (連続した配列作成)

修正前

$expected = [0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23];

修正後

$expected = range(0, 23);

こちらのほうが可読性が高く、意図も伝わりやすく、ミスも起こりづらいです。

参考: PHPマニュアル range

2. $array[] = "aa" (要素を末尾に追加)

修正前

array_push($array, "aa");

修正後

$array[] = "aa";

要素１つを追加するだけなら、こっちのほうが早いし、可読性も高いです。

参考: PHPマニュアル array_push

3. BOM 撤去は fgetcsv() 前で

修正前

$labels = fgetcsv($fh, 0, ",");
$this->__stripUtf8Bom($labels[0]); //UTF-8 の BOM を撤去する

fgetcsv の結果でBOMチェックしてますが、BOMはこのようには判定すべきではありません。

これだと、ラベルの最初の列が "label 1" のように " で囲まれていた場合、
$labels[0] には [BOM]"label 1" のように " が解釈されていない値が入ってしまって、意図通りには動きません。

fgetcsv は通常、文字コードレイヤーの問題は解決済みのファイルを処理することが前提になっているからです（つまり fgetcsv 後に文字コードに関する情報を扱うのは正しくないです）。

修正後

//一旦開いてBOM撤去
$contents = file_get_contents($path);
if ($this->__stripUtf8Bom($contents)) { //UTF-8 の BOM を撤去したらtrueを返す
    fclose($fh);
    $path2 = tmpfile();
    file_put_contents($path2, $contents);
    $fh = fopen($path2);
}
$labels = fgetcsv($fh, 0, ",");

file_get_contents を使っているのでファイルサイズが大きくなる可能性がある場合は、この例の通りにはできませんので気をつけてください。

4. テストの後始末は必ず tearDown() で

修正前

public function testファイルを作成してテストするメソッド() {
    $path = "/tmp/hoge_test.dat";
    $fh = fopen($path);
    $result = $this->Hoge->hogehoge($fh);
    $this->assertTrue($result);
    fclose($fh);
    unlink($path); //テストで作成したファイルを消す
}

これだと、 $this->Hoge->hogehoge($fh) が例外を起こした場合や $this->assertTrue($result) でエラーとなった場合には fclose($fh) や unlink($path) が走りません。

修正後

public function tearDown() {
    if (!empty($this->fh)) {
        fclose($this->fh);
        $this->fh = null;
    }
    if (!empty($this->path)) {
        unlink($this->path);
        $this->path = null;
    }
}

public function testファイルを作成してテストするメソッド() {
    $this->path = "/tmp/hoge_test.dat";
    $this->fh = fopen($this->path);
    $result = $this->Hoge->hogehoge($this->fh);
    $this->assertTrue($result);
}

こうすることで、エラーが発生しても、確実に後始末をすることができます。

5. PHPDoc のコメントには型宣言を

修正前

/**
 * ○○を返す
 * @param $aa       aaを渡す
 * @return エラーメッセージ or string で○○
 */

PHPStorm で開発していると、PHPDoc にかかれた型で、静的な文法チェックやメソッドの補完が可能なのですが、@return の後の最初に登場した単語を型と見なしてしまうので、使用するときに警告が出てうるさいのです。

修正後

/**
 * ○○を返す
 * @param string $aa  aaを渡す
 * @return array|string エラーメッセージ or string で○○
 */

特にライブラリなどを作成する場合には型宣言が欲しいです。

今日は、この辺で。

明日は、「AspectMockについて」ですね！
テスト大好きなので Mock と聞くと、ワクワクが止まりません。