ホーム最近公開の記事

中の人

マジックナンバーはやめてくれ

公開: 2021-12-05 22:04
更新: 2023-04-06 14:56

1504

php クソコードマジックナンバーハードコーディング

頼むわ

【広告】

【スミジョブ】住み込みのお仕事に特化した求人情報サイト

IT専門転職エージェント＠PRO人【アットプロジン】

指示するだけでトランクルームから荷物を配送できる宅トラ

こんにちは！

中の人です。

最近いわゆる「クソコード」が蔓延したプロジェクトを触る機会がありましたので、私自身も気をつけようという意味も込めて目の当たりにしたコードを紹介したいと思います。

マジックナンバーはやめてくれ

さぁ、タイトルで何を言わんとするか予想できていると思いますが、直面したコードは以下のようなコードです。

<?php
...
if($v[26] != 1 AND in_array($v[1], $s)) $hoges[] = $v;
...
$fuga = array_column($hoges, 3);
...
if($v[3] === $vs) $piyos[] = $v;
...
foreach($piyos as $vc) {
	if($val[6] === $vc[0]) {
		echo trim($vc[1]);
		if($vc[1] !== '' AND $vc[2] !== '') echo '、';
		if($vc[2] !== '') echo trim($vc[2]);
		....
	}
}

はいキター!!･:*+.\(( °ω° ))/.:+!!

予想を裏切らない素晴らしいコードですね！

このコードはjsonファイルとcsvファイルを読み込んで、その両方の値を比較したり、出力したりうんぬんかんぬんという処理が書かれていたのですが、見ての通りどちらのファイルも読み込みっぱなしで、必要なデータをindex(0とか1とか)で指定しているので、一体なんの値を参照しているのかわかりませんよね。

ただ、csvファイルはheaderがあるものだったので、わかったのですが、問題はjsonファイルで、プログラムで作成されるファイルだし、keyはindexだし、項目が50個近くまであるし、しかもデータによって項目の数が違うという仕様で、ひたすらカオスでした。

ちなみに、このように書いた本人しかわからない数字などはマジックナンバーと呼ばれています。

参考: マジックナンバー（magic number）とは - IT用語辞典 e-Words

実際のコードでは、要所要所に

◯◯取得処理

のようなコメントが残されていて、ある程度予想できる変数名が書かれている箇所も有りましたが、肝心のデータのkeyの説明が一切書かれていなかったので、「keyの説明を頂戴!!」と心の中で叫びながらjsonファイルを作成する処理からデバッグしてコードの解析をしました。

で、こういうコードの何が悪いかというと、書いているとき、書いている本人はデータの内容がわかっているのでいいんですが、いざこのコードを他人が見たとき、一気に謎コードになるところです。

さらに、書いた本人がずっと記憶してくれていれば、まぁ百歩譲っていいのですが、半年、一年と経った後に、書いた本人ですらわからなくなる可能性があります。

少なくとも、私は絶対に忘れます！

せめてkeyは振り直してくれ

こういったコードの正解は正直わかりません(極論ない)が、私は面倒でもkeyを振り直します。なぜかというと、この後の処理が格段に分かり易くなるからです。

public function setKey(array $row): array
{
    $array = [];
    foreach ($row as $index => $value) {
        switch ($index) {
            case 0:
                $array['name'] = $value;
                break;
            case 1:
                $array['email'] = $value;
                break;
            .....
        }
    }
    return $array;
}

どこかしらに一箇所でもこのような処理があれば、データの配列の中身全体が判明、もしくはある程度の予想が立ちますよね。これは書いている本人もそうですし、他人からしてみてもそうではないでしょうか？

ということで、誰かの手に渡ってもスムーズに改修が開始できるようにkeyを振り直す、または面倒ならindexを定数化もしくはコメントだけでいいのでデータ配列の説明を書いてくれということです。

ハードコーディングがあちこちにあるんじゃ

📁 root/functions/function.php

<?php
...

function hoge_fuga_201901($f)
{
    $d = [];
    if ($f == '/hoge/fuga') {
        $json = file_get_contents($_SERVER[ 'DOCUMENT_ROOT' ]."/dev/hoge/fuga/piyo/baz.json");
        .....
    } else {
        $csv_path = $_SERVER[ 'DOCUMENT_ROOT' ].$f.".csv";
        .....
    }
    return $d;
}

キテるキテる！！

今俺最高にキテる！＼＼\\٩( 'ω' )و //／／！

(ちなみにハードコーディングとは、処理の中に決め打ちのpathなどがそのまま記載されていることをいいます。

参考: ハードコード（オンコーディング）とは - IT用語辞典 e-Words

)

はい、皆さんと一緒に見ていこうと思います。

ハードコーディングとは関係ないですが、そもそも

function hoge_fuga_201901($f)

こちらですが、実際は

function get_csv_201901($f)

このような名前の関数でした。

毎年１月に開催されるcsvファイルをゲットするための大規模イベントみたいな関数名ですね！ナイスです！私もこれから毎年参加しよう思います！大量csvファイルゲットだぜ！

さらに「get_csv」と書いてあるのに、if文で分岐して取得するファイルは、json、csvの2種類のファイルが指定されています。jsonファイルもゲットできるぜ！

if ($f == '/hoge/fuga') {

見事なハードコーディングです。それにしても「$f」てなんのfだ？f◯ck？f◯ckのfを言ってんの？いや流石にそれはないでしょ先輩！あ、folderのfか！いや、fileのf?もうわからないからf◯ckのfにしておこう！f◯ckのfであってました！先輩！

さらにこのハードにコーディングされた文字列、わざわざ一致で条件分岐しているにも関わらず、取得するjsonファイルのパスの中にはない文字列なんだぜ！ならnullとかでよくないか!?ナイス引数です！

　　　　　　　　if ($f == '/hoge/fuga') {
        $json = file_get_contents($_SERVER[ 'DOCUMENT_ROOT' ]."/dev/hoge/fuga/piyo/baz.json");
        .....
    } else {
        $csv_path = $_SERVER[ 'DOCUMENT_ROOT' ].$f.".csv";
        .....
    }

ハードコーディングはもうお腹いっぱいなのでもういいですが、

$json = file_get_contents($_SERVER[ 'DOCUMENT_ROOT' ]."/dev/hoge/fuga/piyo/baz.json");

この

..."/dev/...

この部分は、テスト環境と本番環境で違う値が入る仕様となっていました。

つまり、テスト環境ではこのまま

dev

でオッケーですが、本番環境のコードは、この部分が

prod

になる感じです。

このように環境によって文字列を書き換えなければいけないファイルがいくつもありました。

うーんとてもナイスです！！全裸監督！助けてくれ!!

Configファイルを作ってくれ

こういうハードコーディングする箇所はconfigファイルを作成して定数化してまとめましょう。

📁 root/config.php

<?php

const ROOT = __DIR__ . '/..';

// テスト環境->dev,本番環境->prod
const TARGET_DIR = '/dev';

const HOGE_FUGA = '/hoge/fuga';

const BAZ_JSON_PATH = ROOT.TARGET_DIR.'/hoge/fuga/piyo/baz.json';

...

<?php

require_once(__DIR__.'/../config.php');　

function hoge_fuga_201901($path)
{
　　　　　　　if ($path === HOGE_FUGA) {
        $json = file_get_contents(BAZ_JSON_PATH);// root/dev(prod)/hoge/fuga/piyo/baz.json
        .....
    } else {
        $csv_path = ROOT.$path.'.csv';// root/hoge/fuga.csv
        .....
    }
}

本来はjsonファイルを取得する関数、csvを取得する関数と分けたいところですが、もうこの関数はどこでどのように利用されているか探すのが面倒ですし、実際に動いている処理なので関数名はこのままにすることにして、引数を予想できる変数名に変更、定数化できる箇所はconfigファイルに定数化し、そこから参照するようにします。

ちょっとROOTの指定の仕方が最後に「/」がないので違和感ありますが、まぁしょうがないですね。

このようにすると、少しは関数内が整理され見易くなりますし、ハードコーディングをconfigファイルに定数化したことで、テスト環境と本番環境で変更が必要になるファイルがconfigファイルのみとなりますので、当然、修正漏れも発生しにくくなります。

で、configファイルにコメントが記載されていますが、このように書いておいてくれると、本番環境では、「dev」の部分を「prod」にすればいいんだと、他人が見て一目瞭然になります。

ということで、環境によって変わる可能性があるパラメータや、いろんなファイルで使う可能性のあるパラメータは設定ファイルを作成してそこから参照するようにして欲しいということです。

また、他人が間違えそうなパラメータなどは、コメントもしくはreadmeとかに書いてドキュメント化してほしいということです。