気付いたらまた追記などしたりするんだろうなぁ、とか思いつつ。
とりあえず現状気付いた「美しくないっていうかぶっちゃけ醜い」と感じるソースの傾向について。
もちろん、プログラムの美醜については色々な見解見地があると思うのですが。
基本的には…
・当たり前のことが出来ていない
という、割とみもふたもない一点に尽きるように思います。
もちろん「新しい技術」を得ることは大切なのですが、同じくらいに「得たはずの、当たり前の知識」がちゃんと実践できているかを、もう一度確認してみませんか？
諸悪莫作衆善奉行。
よいことをしなされ。わるいことはしなさんな。
五歳の童でも知っているが、八十の翁にしてなお行い難し。
そんな内容に気付いてもらえたらうれしいなぁ、ってのが、大まかな趣旨でやんす。
コメントがない：超深刻
大抵「ろくでもないソース」です B-p
警戒レベルを２〜３段跳ね上げましょう。
コメントの要不要に関するおいちゃんの見解は「コメントは補足説明 http://d.hatena.ne.jp/gallu/20101116/p1 」をご覧ください。
「コメントは不要」という意見に対して、おいちゃんは半歩たりとて引きません。
コメントがろくでもない：深刻
よく笑い話に出てくる「変数iに１を足す」の類。
「何のためにコメントを書くのか」を小一時間ほど。
まぁ「とりあえずコメントは書いてある」ので、その質をまともにすると、大分とよくなるのですが。
共通化ができていない：超深刻
「共通化が必要か不要か」についての議論は最早「火を見るより明らか」かとは思うのですが*1。
実際に業務に入ると…まぁ「出来ない」「やれてない」「中途半端」 orz
どれくらい綺麗に「共通化できるか」は、割と本気で「技術レベルの一つの指標」になるので。この部分は、特に丁寧に突っ込んでいきたいか、と。
過去に「複数回」拝見した、栄誉ある状況を、思い出せる限りにおいて列挙してみましょう。
ちなみに、大抵のケースにおいて、テキストエディタの「コピー＆ペースト」機能が大活躍しています。…いやまじで、コピペ禁止テキストエディタとか作るとよいんじゃなかろうか？ って、真剣に思うことが多々あります。
ifの連打：超深刻
似たような処理のifが縦に並ぶとかいうケースが、まぁちらほらと。
if ($param1 == 'test') {
$なんちゃら->メソッド('test');
} else
if ($param1 == 'hoge') {
$なんちゃら->メソッド('hoge');
} else
if ($param1 == 'foo') {
$なんちゃら->メソッド('foo');
} else
if ($param1 == 'bar') {
$なんちゃら->メソッド('bar');
} else
if ($param1 == 'hogebar') {
$なんちゃら->メソッド('hogebar');
} else
if ($param1 == 'boo') {
$なんちゃら->メソッド('boo');
}…なにしたいんだろう？
せめて最後にelse句があればかろうじて「許可された文字列かそれ以外かを判定したいのかなぁ」と、わずかながらに思わなくもないけど。
上述なら素直に
$なんちゃら->メソッド($param1);
で終わり。
これで問題がある場合(大抵は引数の存在チェックをしたい)、
// 許可対象の一覧
// XXX ここにおくかconfigにおくか別の場所におくかは、それもまた一つ重要な議論
$checked_string_array = array(
'test' => 1,
'hoge' => 1,
'foo' => 1,
'bar' => 1,
'hogebar' => 1,
'boo' => 1,
);
// ほげほげな処理を行う
if (true === isset($checked_string_array[$param1])) {
$なんちゃら->メソッド($param1);
} else {
// XXX 元プログラムには存在していないエラー処理
}hashのkeyのほうにチェックしたい文字列を持ってきてるのは、検索しやすいようにするため。値にもっちゃうと線形探索なロジックになるので、おいちゃん的にお好みではない。
同一処理の連打：超深刻
似たようなもん。
実際にあって、まぁなんていうかどんびいたものでございます。
$printData['test'] = htmlspecialchars($_GET['test']); $printData['hoge'] = htmlspecialchars($_GET['hoge']); $printData['foo'] = htmlspecialchars($_GET['foo']);
面倒だから３行しか書いてないけど、実際には十数行あったと記憶してる orz
これは(これも)loopを使って小さくまとめましょう。
// えすけぷ処理対象の一覧
// XXX ここにおくかconfigにおくか別の場所におくかは、それもまた一つ重要な議論
$target_names = array(
'test',
'hoge',
'foo',
);
// ぶん回して一気に処理
foreach($target_names as $name) {
// えすけーぷする
$printData[$name] = htmlspecialchars($_GET[$name]);
}
せめてこの程度は、最低でも。
＆
上述の「頭痛が痛かった」実例。
$this->get_conv()->monoDic("customer_id", security::sanitize_html($customer_data["customer_id"]));
$this->get_conv()->monoDic("name1", security::sanitize_html($customer_data["name1"]));
$this->get_conv()->monoDic("name2", security::sanitize_html($customer_data["name2"]));
$this->get_conv()->monoDic("name1_kana", security::sanitize_html($customer_data["name1_kana"]));
$this->get_conv()->monoDic("name2_kana", security::sanitize_html($customer_data["name2_kana"]));
$this->get_conv()->monoDic("email", security::sanitize_html($customer_data["email"]));
$this->get_conv()->monoDic("uid", security::sanitize_html($customer_data["uid"]));
$this->get_conv()->monoDic("sex_name", security::sanitize_html($sex_name));
$this->get_conv()->monoDic("birthday", security::sanitize_html($customer_data["birthday"]));
$this->get_conv()->monoDic("birthday_year", security::sanitize_html($birthday_year));
$this->get_conv()->monoDic("birthday_month", security::sanitize_html($birthday_month));
$this->get_conv()->monoDic("birthday_day", security::sanitize_html($birthday_day));
$this->get_conv()->monoDic("zip1", security::sanitize_html($customer_data["zip1"]));
$this->get_conv()->monoDic("zip2", security::sanitize_html($customer_data["zip2"]));
$this->get_conv()->monoDic("prefecture_name", security::sanitize_html($prefecture_name));
$this->get_conv()->monoDic("city", security::sanitize_html($customer_data["city"]));
$this->get_conv()->monoDic("address", security::sanitize_html($customer_data["address"]));
$this->get_conv()->monoDic("building", security::sanitize_html($customer_data["building"]));
$this->get_conv()->monoDic("point", security::sanitize_html($customer_data["point"]));
$this->get_conv()->monoDic("temp_point", security::sanitize_html($customer_data["temp_point"]));
$this->get_conv()->monoDic("memo", security::sanitize_html($customer_data["memo"]));こんなのもあった。…いやまぁこっちはそもそもとしてあちこち論外なのだけれども。…せめて「書かざるを得ない」んなら、loopにしようよ、と orz
if($_POST["a"]){$a = $_POST["a"];}elseif($_REQUEST["a"]){$a = $_REQUEST["a"];}elseif($_GET["a"]){$a = $_GET["a"];}
if($_POST["b"]){$b = $_POST["b"];}elseif($_REQUEST["b"]){$b = $_REQUEST["b"];}elseif($_GET["b"]){$b = $_GET["b"];}
if($_POST["c"]){$c = $_POST["c"];}elseif($_REQUEST["c"]){$c = $_REQUEST["c"];}elseif($_GET["c"]){$c = $_GET["c"];}
if($_POST["d"]){$d = $_POST["d"];}elseif($_REQUEST["d"]){$d = $_REQUEST["d"];}elseif($_GET["d"]){$d = $_GET["d"];}
if($_POST["e"]){$e = $_POST["e"];}elseif($_REQUEST["e"]){$e = $_REQUEST["e"];}elseif($_GET["e"]){$e = $_GET["e"];}
if($_POST["f"]){$f = $_POST["f"];}elseif($_REQUEST["f"]){$f = $_REQUEST["f"];}elseif($_GET["f"]){$f = $_GET["f"];}
if($_POST["g"]){$g = $_POST["g"];}elseif($_REQUEST["g"]){$g = $_REQUEST["g"];}elseif($_GET["g"]){$g = $_GET["g"];}
if($_POST["h"]){$h = $_POST["h"];}elseif($_REQUEST["h"]){$h = $_REQUEST["h"];}elseif($_GET["h"]){$h = $_GET["h"];}
if($_POST["i"]){$i = $_POST["i"];}elseif($_REQUEST["i"]){$i = $_REQUEST["i"];}elseif($_GET["i"]){$i = $_GET["i"];}
if($_POST["j"]){$j = $_POST["j"];}elseif($_REQUEST["j"]){$j = $_REQUEST["j"];}elseif($_GET["j"]){$j = $_GET["j"];}
if($_POST["k"]){$k = $_POST["k"];}elseif($_REQUEST["k"]){$k = $_REQUEST["k"];}elseif($_GET["k"]){$k = $_GET["k"];}
if($_POST["l"]){$l = $_POST["l"];}elseif($_REQUEST["l"]){$l = $_REQUEST["l"];}elseif($_GET["l"]){$l = $_GET["l"];}
if($_POST["m"]){$m = $_POST["m"];}elseif($_REQUEST["m"]){$m = $_REQUEST["m"];}elseif($_GET["m"]){$m = $_GET["m"];}
if($_POST["n"]){$n = $_POST["n"];}elseif($_REQUEST["n"]){$n = $_REQUEST["n"];}elseif($_GET["n"]){$n = $_GET["n"];}
if($_POST["o"]){$o = $_POST["o"];}elseif($_REQUEST["o"]){$o = $_REQUEST["o"];}elseif($_GET["o"]){$o = $_GET["o"];}
if($_POST["p"]){$p = $_POST["p"];}elseif($_REQUEST["p"]){$p = $_REQUEST["p"];}elseif($_GET["p"]){$p = $_GET["p"];}
if($_POST["q"]){$q = $_POST["q"];}elseif($_REQUEST["q"]){$q = $_REQUEST["q"];}elseif($_GET["q"]){$q = $_GET["q"];}
if($_POST["r"]){$r = $_POST["r"];}elseif($_REQUEST["r"]){$r = $_REQUEST["r"];}elseif($_GET["r"]){$r = $_GET["r"];}
if($_POST["s"]){$s = $_POST["s"];}elseif($_REQUEST["s"]){$s = $_REQUEST["s"];}elseif($_GET["s"]){$s = $_GET["s"];}
if($_POST["t"]){$t = $_POST["t"];}elseif($_REQUEST["t"]){$t = $_REQUEST["t"];}elseif($_GET["t"]){$t = $_GET["t"];}
if($_POST["u"]){$u = $_POST["u"];}elseif($_REQUEST["u"]){$u = $_REQUEST["u"];}elseif($_GET["u"]){$u = $_GET["u"];}
if($_POST["v"]){$v = $_POST["v"];}elseif($_REQUEST["v"]){$v = $_REQUEST["v"];}elseif($_GET["v"]){$v = $_GET["v"];}
if($_POST["w"]){$w = $_POST["w"];}elseif($_REQUEST["w"]){$w = $_REQUEST["w"];}elseif($_GET["w"]){$w = $_GET["w"];}
if($_POST["x"]){$x = $_POST["x"];}elseif($_REQUEST["x"]){$x = $_REQUEST["x"];}elseif($_GET["x"]){$x = $_GET["x"];}
if($_POST["y"]){$y = $_POST["y"];}elseif($_REQUEST["y"]){$y = $_REQUEST["y"];}elseif($_GET["y"]){$y = $_GET["y"];}
if($_POST["z"]){$z = $_POST["z"];}elseif($_REQUEST["z"]){$z = $_REQUEST["z"];}elseif($_GET["z"]){$z = $_GET["z"];}
ifでエラー処理の書式の連打：行飛びだから意外と気付かない：深刻
これは少し趣が違う。
エラー系処理なんかでよく見るかなぁ。
if(とあるエラー) {
header('Location: ./error1.html');
exit;
}
if(とあるエラー2) {
header('Location: ./error2.html');
exit;
}
if(別のエラー) {
header('Location: ./error_hogehoge.html');
exit;
}
if(びっくりするエラー) {
header('Location: ./error_pamyupamyu.html');
exit;
}間に挟まる処理にもよるんだけど、とりあえず「エラーで突っ返すlocation」は一箇所のほうが、後々楽なので、まとめましょう。
他の処理がはさからない(或いはエラーに因らない処理しかはさまらない)場合は、こんな書式。
$error_location = '';
if(とあるエラー) {
$error_location = './error1.html';
}
if(とあるエラー2) {
$error_location = './error2.html';
}
if(別のエラー) {
$error_location = './error_hogehoge.html';
}
if(びっくりするエラー) {
$error_location = './error_pamyupamyu.html';
}
// エラー判定
if ('' !== $error_location) {
// エラーらしいので終了しておく
header('Location :' . $error_location);
// XXX 使ってる状況FWにも因るけど、できるだけexitぢゃなくてreturnで処理をもどしましょう
return ;
}
こんな感じ。
もし「細かくエラー判定をして成功への階段をいっぽづつ踏みしめる」系の場合、例外処理使うのが多分早い。
ソースは省略。リクエストがあったら捏造します(笑
「インタフェース/処理のわずかな違い」をコピペで量産：超深刻
以下の２つのプログラムには７つ未満の差異があります。どこでしょう？
protected function dice($num)
{
//
$ret = array();
$ret['total'] = 0;
$ret['malice'] = 0;
$ret['dice'] = array();
//
for($i = 0; $i < $num; $i ++) {
$d = mt_rand(1,6);
$ret['total'] += $d;
$ret['dice'][] = $d;
//
if (6 === $d) {
$ret['malice'] ++;
}
}
//
return $ret;
}
protected function dice2($num)
{
//
$ret = array();
$ret['total'] = 0;
$ret['malice'] = 0;
$ret['dice'] = array();
//
for($i = 0; $i < $num; $i ++) {
$d = mt_rand(1,6);
$ret['total'] += $d;
$ret['dice'][] = $d;
//
if (1 === $d) {
$ret['malice'] ++;
}
}
//
return $ret;
}
メソッド名が違うことを含めても、ぶっちゃけると「２箇所しか違いません」。
こーゆーことをやられると、なんていうか割と本気で「闇よりもなお暗きもの 夜よりもなお深き存在 混沌の海よ たゆたいしもの 金色なりし闇の王...」とかって詠唱を始めたくなってしまう。
上述は「判定値が一つ違う」のですが。他にも「引数が１つ違うだけ」とか「SQL１文だけ少し違う」とかもうちょっと酷いと「見にいくテーブルが違うけどカラム名は同じだからつまり"テーブル名が違うだけ"」とか。
ンなことをやるからソースが膨れ上がり、わけわかめになるのです。
メソッド名を含めて、考え直しましょう。上述はおおむね「dice2が後から追加された」のが見え見えなので。dice2が追加されるタイミングで、脊髄反射で「共通化」できるようにしておきましょう。
「オレカコイイ！」実装：深刻〜超深刻
出来るようになった楽しい気持ちはわかるのですが、業務ではやめておきましょう。
習作でやる分には全然OKだと思うんだけど。
複雑な正規表現
まぁそのまんま。「どこまでを複雑とするか」って議論はあるのだけれども、そこも含めて意識をしておくとベターです。
流儀ドン無視：超深刻
別段、現場の流儀が「常に正しい」とはまったく思わないですし、場合によっては「神速の突っ込みをいれないとまずい」流儀も多々存在します。
でも「流儀に対して意見を申し述べる」のと「流儀を聞きもせずに横紙破りに破る/無視する」のは、別次元のお話です。
「俺の使いたいツールはすべて使えるべきだ！」：深刻
書いといてなんですが、結構難しい問題です(苦笑
ただとりあえず。現場さんに伺ったときは、まずすべてのツールについて「確認」は取ったほうがよいと思います。
使ってから「これ使ってるんで」で事後承諾にすると、あまりいい顔をされません。オプトアウトが嫌われるのは、spamに限らないものです。
現場のコーディング規約破り：超深刻
割と論外。
もちろんコーディング規約にもピンキリあるので、場合によっては突っ込むことも大切ですが。
聞きもせずに「オレオレコーディング」すると、大抵、とても嫌な顔をされます。
特にインデント。これを違えると、とても敏感に反応する人が多いので、要注意です。
フレームワークの流儀無視：超深刻
フレームワークは、大抵「導入理由」がありますし、またそこから、その現場が「どんなメリットを享受したいのか」という背景があって、その上に「その現場で、そのフレームワークを使う流儀」というのが成り立ちます。
なので、最低限「FW使用にあたって、に注意を払うべきか」という「意図」の部分は、当然ながらちゃんと聞かないといけませんし、不明点は質問をしなきゃいけません。
特に危ないのが「前の現場でも使ってた(或いはプライベートで使ってる)」フレームワーク。
「同じフレームワーク」だから「同じような意図だろう」と妄想することは、極めて危険です。大きく外して「場外ファール」になって「リカバリ不能」な状態を作ってしまうその前に、きちんとコミュニケーションをとりましょう。
…実例かいてたら案外とでかくなった(笑
また報告発見があったら適宜追加します〜。
