こんにちは!
中の人です。
最近いわゆる「クソコード」が蔓延したプロジェクトを触る機会がありましたので、私自身も気をつけようという意味も込めて目の当たりにしたコードを紹介したいと思います。
マジックナンバーはやめてくれ
さぁ、タイトルで何を言わんとするか予想できていると思いますが、直面したコードは以下のようなコードです。
<?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個近くまであるし、しかもデータによって項目の数が違うという仕様で、ひたすらカオスでした。
ちなみに、このように書いた本人しかわからない数字などはマジックナンバーと呼ばれています。
実際のコードでは、要所要所に
のようなコメントが残されていて、ある程度予想できる変数名が書かれている箇所も有りましたが、肝心のデータの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などがそのまま記載されていることをいいます。
)
はい、皆さんと一緒に見ていこうと思います。
ハードコーディングとは関係ないですが、そもそも
function hoge_fuga_201901($f)
こちらですが、実際は
function get_csv_201901($f)
このような名前の関数でした。
毎年1月に開催される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とかに書いてドキュメント化してほしいということです。
終わりに
多分このコードを書いた方も頑張って書いたんだと思います。コードを見ればなんとなくわかります。度重なる仕様変更、追加される要望、迫る納期と戦った結果だと思います。なので一概に文句を言うのは、その時の状況を知った上で言うべきでしょう。でも、やっぱり、何じゃこのクソコードはぁぁぁ!!て思いますけどね!
まぁ、私も人のこと言えませんが。
ということで、こういうコードに触れて、自分を戒めてレベルアップしていきたいなと言うことで!
ではまた!