レガシーコードの危険な罠：見落とされやすい致命的バグの解説 • AQZ Tech Chronicles

今回は、実際のレガシーコードを題材に、一見問題なく見えるコードに潜む致命的なバグについて解説します。

問題のあるコード

$sql10 = $pdo->prepare("SELECT * FROM define_rankmonth");
$chk10 = $sql10->execute();
$data10 = $sql10->fetch(PDO::FETCH_ASSOC);
$tmonth = $data10["tmonth"];
if(empty($tmonth)){
    $tmonth = 6;
}

//直近一ヶ月の売上総額、購入頻度を
$today = date("Y-m-d");
$monthbefore = date("Y-m-d",strtotime("-".$tmonth." month"));

$basesql_clientinfo = "select *,
          (select count(0) from followinfo where followinfo.clid=clientinfo.clid) as followcount,
          (select sum(pronum*proprice) from salespro where salespro.clientid=clientinfo.clid
           and salesdate>='".$onemonthbefore."' and salesdate<='".$today."') as sumsalespro,
          (select count(0) from salesinfo where salesinfo.saclid=clientinfo.clid
           and sacldate>='".$onemonthbefore."' and sacldate<='".$today."') as salescount
          from clientinfo";

この20行ほどのコードにいくつもの問題が潜んでいるのです。あなたは分かりますか？

致命的な問題点

意味不明な変数名と不明確な仕様

$sql10 = $pdo->prepare("SELECT * FROM define_rankmonth");
$chk10 = $sql10->execute();
$data10 = $sql10->fetch(PDO::FETCH_ASSOC);
$tmonth = $data10["tmonth"];
if(empty($tmonth)){
    $tmonth = 6;
}

このコードには複数の深刻な問題が存在します。

まず最も深刻な問題は変数名の付け方です。 $sql10、$chk10、$data10という変数名からは、その目的や役割を全く理解することができません。数字の「10」の意味も不明確です。おそらく他にも$sql1から$sql9まで存在するのか、あるいは単なる任意の番号なのか、その意図を読み取ることができません。このような命名規則は、コードの保守を著しく困難にするだけでなく、デバッグ時の原因特定も遅らせることになります。

さらに問題なのは$tmonthという変数名です。この「t」が何を意味するのか、コードを読んでも理解することができません。「target」なのか「total」なのか「temporary」なのか、様々な可能性が考えられます。実際の開発現場では、この「t」の意味を理解しようとして時間を無駄にすることになり、最終的には諦めて、単に「期間を表す変数」という曖昧な理解に留めるしかありません。

最も重大な問題として、デフォルト値として設定されている「6」（6ヶ月）の根拠が全く示されていない点が挙げられます。この値がなぜ6ヶ月なのか、誰がこの値を決定したのか、どのような業務的な判断があったのか、そしてこの値を変更した場合の影響範囲がどこまで及ぶのか、これらが一切不明です。もし業務要件の変更でこの値を変更する必要が出てきた場合、その影響を予測することができず、安全な変更が困難になります。

コメントと実装の危険な不一致

このコードではコメントに「直近一ヶ月の売上総額、購入頻度を」と書かれています。しかし実際の実装では$tmonthという変数が使用されており、この変数のデフォルト値は6となっています。このような不一致は、コードの動作に関する重大な誤解を招きます。たとえば、売上集計やレポート作成において、期待される1ヶ月分のデータではなく、6ヶ月分のデータが使用されることになります。その結果、経営判断に使用される数値が大きく歪められる可能性があります。

ひどいことに設計書も仕様書も残されていないため、何が正しいのかもわかりません。

未定義変数による実行時エラー

SQLクエリ内で使用されている$onemonthbeforeという変数は、コード内のどこにも定義されていません。定義されているのは$monthbeforeという似て非なる変数名です。

なぜ、こうなったのか？カギとなるのは直前のコメント「//直近一ヶ月の売上総額、購入頻度を」です。間違ったコメントを読み、一か月間を指定する変数をSQLクエリに渡すべきだと誤解したのかもしれません。

いずれにしろ、この問題により、日付の比較対象が”になるため、MySQL8では実行時エラーとなり、データを取得できません。（※MySQL5.7では日付にNULLが許されていた。）

日付計算の致命的な問題

月の計算に関して、このコードには深刻な問題があります。例えば、2024年3月31日から1ヶ月前の日付を計算する場合を考えてみましょう。

$today = "2024-03-31";
$monthbefore = date("Y-m-d", strtotime("-1 month", strtotime($today)));
// 結果：2024-03-03

このコードは2024年2月31日（存在しない日付）を計算しようとし、 PHPの自動補正により3月3日という予期せぬ日付になってしまいます。この問題は月末日付近のデータ集計において、データの欠落や重複を引き起こす可能性があります。

私は経営コンサルタントもしており、マーケティングも手がけています。

一般的にマーケティング等においてnカ月前という計算を行う際、月別に集計をします。例えば2024年11月12日から半年間のデータを集計する場合、 6～11月のデータを集計する必要があります。つまり、6月1日から11月12日までのデータを集計するのが一般的なのですが、要件定義書も基本設計書も残されておらず、コメントも書いてないため、そもそもの要件が分かりません。

プログラマーの裁量というか、推量でコードを書いていると思われますが、これではビジネスロジックの正確性を担保することができないように、個人的には思います。

ですが、今まで特にクレームもなかったので、違和感を感じつつも現状の仕様を維持し、本来の意図と計算が違っていると思われる、月末の日付だけ修正することがベターかな、と判断するしかありません。

改善策の提案

日付計算の適切な実装

function getPeriodStart($endDate, $months) {
    $date = new DateTime($endDate);
    $day = $date->format('d');

    // 月初めに移動してから月数を減算
    $date->modify('first day of this month');
    $date->modify("-" . ($months - 1) . " month");

    // 可能な限り同じ日付に移動
    $lastDay = $date->format('t');
    $targetDay = min($day, $lastDay);
    $date->setDate($date->format('Y'), $date->format('m'), $targetDay);

    return $date->format('Y-m-d');
}

変数名と処理の明確化

/**
 * 顧客ランク判定のための集計期間を取得
 *
 * テーブル: define_rankmonth
 * カラム: tmonth - 集計対象月数
 *
 * デフォルト値(6ヶ月)の根拠：
 * - 2023年のマーケティング部門との協議により決定
 * - 季節変動の影響を平準化するため、半年を基準とする
 * - 関連チケット: PROJ-2023-123
 *
 * @return int 集計対象月数
 */
function getRankAnalysisPeriod(): int {
    $rankSettingQuery = $pdo->prepare("SELECT tmonth FROM define_rankmonth");
    $rankSettingQuery->execute();
    $rankSetting = $rankSettingQuery->fetch(PDO::FETCH_ASSOC);

    // デフォルト値の定数化と理由の明記
    const DEFAULT_ANALYSIS_PERIOD = 6; // 半年：季節変動の平準化のため

    return $rankSetting['tmonth'] ?? DEFAULT_ANALYSIS_PERIOD;
}

// ランク計算期間を取得
$analysisPeriodMonths = getRankAnalysisPeriod();

// 集計期間の開始日と終了日を設定
$periodEndDate = date('Y-m-d');
$periodStartDate = getPeriodStart($periodEndDate, $analysisPeriodMonths);

SQL実行の安全性確保

集計用のSQLは、サブクエリの代わりに適切なJOINを使用し、プリペアドステートメントでパラメータを処理するべきです。また、必要なカラムのみを明示的に指定することで、不要なデータ転送を防ぐことができます。

ですが、稼働しているSQL文を変更するのはリスクが高いため、今回は見逃します。負の遺産は一度に下ろさなくてもいいのです。

まとめ

レガシーコードの改善において最も重要なのは、動作の正しさとセキュリティの確保です。特に日付計算のような一見単純に見える処理でも、考慮すべき例外ケースが多く存在することを忘れてはいけません。コードレビューでは、コメントと実装の整合性、変数定義の確認、日付計算の妥当性といった点を重点的にチェックする必要があります。

これらの問題を防ぐため、静的解析ツールの導入や、厳格な命名規則の適用、そして定期的なコード品質レビューの実施が推奨されます。また、日付計算のような複雑な処理は、十分にテストされた専用のライブラリやヘルパー関数を使用することで、信頼性を高めることができます。

たった20行のコードに潜む数々の問題点を解説しましたが、これらの問題は実際の開発現場でもよく見られるものです。コメントや変数名を適切につけ、関数化するだけでも、コードの可読性は飛躍的に向上します。

皆さんも、誰かが自分のコードを読むことを想定して、常に丁寧なコーディングを心がけましょう。