後輩にレビューで突っ込みを入れた話 JavaScript編

#はじめに
ここは後輩に指導？助言？したコードレビューをつらつらと書いていたり。
とりあえずJavaScript編を適当に。もっといい実例とか修正先あったら教えてください。

##そのif文要るの？編
私的にはパフォーマンス云々を抜きにして、if文をできるだけ回避して書いた方がいいと思ってるので、そういうところよく見てみたりします。特に多重ネストはやめようよと。
###しょにょ１

1_before.js

if (!muted){
	if (volume > 0.05) {
		volume = volume - 0.05;
		$("#volume").slider('value',volume);
	} else {
		$("#volume").slider('value',0);
		volume = 0;
	}
}

要するに今の音量から0.5引いて、ゼロより小さかったら0にするって処理。
…コレ単に矯正するだけならif文で分ける必要ないよね？
そもそもなんで順番逆なんだ？そういうミスをしないためにも、if文使わずにやりましょうって教えた修正。

1_after.js

if (!muted){
	volume = Math.max((volume - 0.05),0);
	$("#volume").slider('value',volume);
}

５行縮まる上、条件分岐も要らないしこれでいいのでは的な。
１行１処理にするとしても、変数hogeに計算結果入れてMath.max(hoge,0)で２行。都合４行節約。
言うに及ばず、上限でも同じようなことしてやがったのでそっちも直すことにより効果は倍となりますた。

###しょにょ２
いわゆるゼロ補完。

2_before.js

if (second < 10){
	return "0" + second;
}else{
	return second;
}

これは個人的に無言で直した。

2_after.js

return ("0" + second).slice(-2);

これは好みが分かれても仕方ないと思ってるけど、僕はif文書く必要はないと思うのよね…。

##君何も考えずにコピペしてるよね編
例え動作の意図が分かってコピペしてたとしても、似たような内容を並べるなというアレ。

###しょにょ３
まず、以前に自分が書いていたコード。

3_inherit.js

$('#hoge_btn').on('click', function(){
	$(this).attr('disabled', true);
	doHoge($(this));
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$(this).attr('disabled', false)},1000);
});

一時的に機能を殺し１秒後に復帰（連打対策）を仕込みつつ処理実施。（当時は）同じ処理機構のない単機能だったのでサクッと書いた。のだが…
（このコード自体、自分で今見るとちょっとアレでソレだけど、今回はそれを棚に上げて）

一か月後、コードを見たら後輩がいろいろ機能を追加してたんだけど、ファイル見て唖然と。

3_before.js

$('#hoge_btn').on('click', function(){
	$(this).attr('disabled', true);
	doHoge($(this));
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$(this).attr('disabled', false)},1000);
});
$('#alice_btn').on('click', function(){
	$(this).attr('disabled', true);
	doAlice($(this));
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$(this).attr('disabled', false)},1000);
});
$('#bob_btn').on('click', function(){
	$(this).attr('disabled', true);
	doBob($(this));
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$(this).attr('disabled', false)},1000);
});
（中略）
$('#zoe_btn').on('click', function(){
	$(this).attr('disabled', true);
	doZoe($(this));
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$(this).attr('disabled', false)},1000);
});

メロスは激怒した。もちろん僕も激怒した。こんなことに忍法コピペ分身の術を使うんじゃない！

3_after.js

$('#hoge_btn').on('click', function(){
	executeBtnEventWithDisable($(this),doHoge);
});
$('#alice_btn').on('click', function(){
	executeBtnEventWithDisable($(this),doAlice);
});
（中略）
$('#zoe_btn').on('click', function(){
	executeBtnEventWithDisable($(this),doZoe);
});
// ボタンイベント処理（ボタンを短期無効化する）
var executeBtnEventWithDisable = function($btn,doFunc){
	$btn.attr('disabled', true);
	doFunc($btn);
	// １秒（＝1000ミリ秒）後に復活
	setTimeout(function(){$btn.attr('disabled', false)},1000);
}

双子までならギリギリ許すが３つから先は纏めろ、と指導しときました。えっ六つ子？当然死すべき。変数名は適当です（後輩の名誉のために補足しておくと、さすがにアルファベット２６個分のセットではなかった）。

#で、なんで後輩はそう書いたの？
聞き取ったところ、おおよそ「そういう発想がなかった」か「そういう発想をする人が今までいなかった」のどちらかに収束するような。書いて動けばそれでいいって現場もあるんだなぁ、と痛感したりとか、僕もそうならんように気を付けないといけんかのぅと。

##そういう発想がなかった
ある標準関数を知らなかっただけで、その関数が実行している内容を遠回りでパワー費やして人月食っちゃう系の事ってあるあるですよねー…
まぁ、何にでもいえることですが、「こういうことができればきれいに書けるのに」と考える癖をつけつつ、それっぽい処理の前例がないか一度当たってみるのがいのかなぁ。
コードレビューのない現場だと、一度動いたら本人以外がいじるケースは少ないからねぇ…不慮の事故とかで居なくなったりすると尚更？

##そういう発想をする人が居なかった
運ゲー
…って単語で他力本願に収束するのは忍びないので何か考えてあげたいと思ったので、「似たこと（＝自分のやろうとしている処理とかそれに近いの）をわざわざネットに書いておいてくれる奇特な心優しいコーダーさんがいないか調べて、コード見比べてみるようにしたら？」という助言をしていおたんだがどうなんだ。

#つづくの？
続いてほしくないけどネタが出たら。