バグを直していて思ったこと

基本的なことだけど自分でもたまに忘れることがある。
そしてバグの元になる。

失敗する可能性のある処理はチェックを行う

<?php
if (isset($_GET['checkbox']) && is_array($_GET['checkbox'])){
    $checkbox = $_GET['checkbox'];
}else
    $checkbox = array();
}
/* foreachの前には必ずチェックを入れる */

foreach ($checkbox as $key => $value){
    ...

ついつい気軽に使ってしまうforeach文。ユーザの入力を使う場合に限らずチェックした方がいい。
配列を返す関数では

<?php
function getGoods($key)
{
    $goods = $this->dao->get($key);
    if (is_array($goods))
        return $goods;
    else
        return array();
}

値が無い場合は空の配列を返す。
他の言語だと普通にやっていることなのにPHPだと守られてない状況が多い気がする。

DB操作も

<?php
/* fruitIdの取得 */
if (!isset($_GET['fruitId']) || !is_numeric($_GET['fruitId'])){
    print("fruitId Error!!\n");
    return;
}

$fruitId = $_GET['fruitId'];


$dbh = new PDO('odbc:sample', 'db2inst1', 'ibmdb2');

/* FRUIT テーブルから一行削除する */
$count = $dbh->exec("DELETE FROM fruit WHERE fruit_id = $fruitId");

/* 削除された行数で処理分岐 */
if (!$count){
    print("Delete Error!!\n"); // 削除を期待している動作で削除されなかった場合、エラーをきちんと報告する
}else{
    print("Deleted $count rows.\n");
}

マニュアルのサンプルを流用するとこんな感じになる。

PHP5なら配列よりもオブジェクト

配列のキーが固定ならクラスのフィールドで定義した方が良い。何度も使われる値は特に。

<?php

/* 配列の場合 */
function sumByArray($goods)
{
    if (isset($goods['price'], $goods['num']))
        return $goods['price'] * $goods['num'];
    else
        return 0;
}

/* オブジェクトの場合 */
class Goods
{
    public $price;
    public $num;

    /* 仕様が変わればここにコメントを書くといい */
}

function sumByObject(Goods $goods)
{
    return $goods->price * $goods->num;
}

配列だとチェックが必要で見づらい。項目が増えてくるとカオスになる。
クラス定義しておくと、項目についての説明をコメントで書いておけばいいので、定義元を見れば仕様が変わっても対応しやすい。

ただ、これは結構面倒なので考えどころ。仕様変更があった時についでに修正するとか、使う場面を考える必要がある。

エラーは必ずメールで飛ばす

今回初めて導入して、効果はかなり大きいことが分かった。
この場合もちろんweb上にはエラーを表示させない。
数人でバグ取りするのと数千人に利用してもらうのは大違いだ。なので、バグが出るのは仕方がないとして、せっかく発見されたバグを逃さないようにしなければならない。
個人的にはログファイルに記録するよりもメールがオススメ。私はエラーをタイトルにして本文にバックトレースと$_REQUESTを書いたメールを送信するようにしている。実害がないバグだからといって放っておくと、どんどんメールが溜まってくるのでやっぱり直さなければという気になる。直したバグは既読メールにするか削除していけば、大きなシステムでも全ての（機能的な、ではなくプログラム的な）バグを取り除くのが容易になる。

致命的なエラーはメール処理に移る前にPHPが停止するので、これは自分たちがデバッグするしかない。

trigger_errorをアサートの代わりにも使える。
例えば

<?php

class OperateLoginUser
{
    const E_NOLOGIN = 1;

    public function someOperate()
    {
        $userId = UserManager::getLoginedUserId();
        if (!$userId){
            trigger_error("ログインしていないユーザがここの処理を通りました", E_USER_WARNING);
            throw new Exception("ログインしてください", self::E_NOLOGIN);
        }

    ...
    }
}