データベースでカンマ区切り(CSV)のデータが入っていることがあります。
例えばこんな感じ（ 6,8,10,12,15,18,19,20 ）のデータです。
カンマ区切りデータは、PHPでよく配列に変換してループを回して処理します。
どの言語でもそうですが、配列やループは処理が遅くなりがちです。
そんな書き方ではいつまでも成長できないので、今回は他人のソースのリファクタリングに挑戦してみました。
今回のコードの前提
現在の時間が、処理すべき時間のリストに入っているか確認して、bool値で返します。
毎時バッチ処理や毎時アラームをイメージしていただければわかりやすいと思います。
リファクタリング前
$hoursList には、カンマ区切りの『時』リストが入ります。例： 6,8,10,12,15,18,19,20
$nowHour には、現在の『時』が入っています。例：15
戻り値は $hoursList 内に $nowHour があれば true 、なければ false です。
function checkHour($hoursList, $nowHour) {
$ret = false;
if ($hoursList == "") {
return $ret;
}
$arrHoursList = explode(",", $hoursList);
foreach ($arrHoursList as $hour) {
if ($nowHour == $hour) {
$ret = true;
}
}
return $ret;
}
リファクタリング結果
1行で書けますね。
function checkHour($hoursList, $nowHour) {
return (strpos(','.$hoursList.',', ','.$nowHour.',') !== false);
}
リファクタリングの過程
改めて、もともとのコードがこちらです。
function checkHour($hoursList, $nowHour) {
$ret = false;
if ($hoursList == "") {
return $ret;
}
$arrHoursList = explode(",", $hoursList);
foreach ($arrHoursList as $hour) {
if ($nowHour == $hour) {
$ret = true;
}
}
return $ret;
}
ループは極力減らす
ループ処理はプログラムの中でも速度が遅くなる元凶です。
ループでの繰り返し処理回数を減らせば減らすほど、プログラムの速度は早くなり無駄なリソースを使いません。
まずは、配列をすべて確認している無意味なコードを修正します。
配列の中で検索しているものがヒットしたのであれば、その場で true を戻し処理を終えるべきです。
function checkHour($hoursList, $nowHour) {
$ret = false;
if ($hoursList == "") {
return $ret;
}
$arrHoursList = explode(",", $hoursList);
foreach ($arrHoursList as $hour) {
if ($nowHour == $hour) {
return true;
}
}
return $ret;
}
変数や配列は最小限にする
変数はプログラムを追跡する際に、常に気にかけてなければならない対象となります。
変数が少なければ少ないほど、コードレビューのときに負担が減ります。
また、配列はメモリ消費量が大きいのでできるだけ使用は控えたいものです。
今回は $ret はなくても動作するので削除します。
function checkHour($hoursList, $nowHour) {
if ($hoursList == "") {
return false;
}
$arrHoursList = explode(",", $hoursList);
foreach ($arrHoursList as $hour) {
if ($nowHour == $hour) {
return ture;
}
}
return false;
}
別のアルゴリズムを検討する
if や for などはプログラムの基本です。
これは私の好みですが、分岐や繰り返し処理は最低限しか使わないほうが好きです。
（特に else は使わなくてもいいケースがほとんど。）
分岐や繰り返しはテストケースの増大も招く恐れもあります。
今回のケースは、文字列の検索で解決できそうです。
例えば、6,8,10,12,15,18,19,20 という時間を表すカンマ区切りの文字列があったとします。
9時が対象になっているかどうかは、9時が上記の文字列の中に入っているかを検索すればよいだけの話なのです。
ここで大事なのは、9 を検索してはいけません。
上記の文字列を見てもらうとわかりますが、19 の 9 がヒットしてしまいます。
,9, のように、カンマで区切られたところを含めて検索すればよいのです。
更に落とし穴があります。上記の検索キーでは先頭と末尾が検索対象から外れてしまいます。
ですので、文字列にもひと工夫してあげます。
6,8,10,12,15,18,19,20 の先頭と末尾にカンマを加えて ,6,8,10,12,15,18,19,20, としてあげるのです。
これで配列に分解してループを回さなくても、同じ判定処理ができるようになります。
また、strpos 関数は文字列が見つからなかったときに false を返す仕様となっています。
https://www.php.net/manual/ja/function.strpos.php
これを利用することで false が返ってきていないなら一律 true と判定することができます。
ついでにガード節も不要となったので削除します。
ループと配列、分岐の処理をなくすことができました。
function checkHour($hoursList, $nowHour) {
return (strpos(','.$hoursList.',', ','.$nowHour.',') !== false);
}
最後に
見直してみると、逆にリファクタリングしすぎですね。。。
あと今回のケースは、できるならテーブルの正規化が正解でしょう！
カンマ区切りのデータが入っているテーブルなんて、非正規化テーブルと同じですから。。。