今回はタブコンテンツのコードレビューを通して、コードが冗長になる原因とシンプルにまとめるコツを模索してみました。
本日コードレビューをするのはAさん、後輩のBさんの作成したタブコンテンツのコードをレビューします。Bさんはゆくゆくはこのタブのスクリプトをプラグイン化したいそうです。
完成形はこのような形になります。
タブが3つあり、そしてコンテンツ内にも別パネルへのナビがついています。
コードレビューの末、Bさんは何とか満足のいくコードを書くことができました。
まずは完成形のコードをご覧ください。
▼HTML
<ul class=”naviWrap”>
<li class=”navi navi01 select”>ナビ01</li>
<li class=”navi navi02″>ナビ02</li>
<li class=”navi navi03″>ナビ03</li>
</ul>
<ul class=”pannelWrap”>
<li class=”pannel01 pannel”>
<p>パネル01です</p>
<ul class=”pannelNavi”>
<li class=”subnavi02 subnavi”>パネル02へ</li>
<li class=”subnavi03 subnavi”>パネル03へ</li>
</ul>
</li>
<li class=”pannel02 pannel”>
<p>パネル02です</p>
<ul class=”pannelNavi”>
<li class=”subnavi01 subnavi”>パネル01へ</li>
<li class=”subnavi03 subnavi”>パネル03へ</li>
</ul>
</li>
<li class=”pannel03 pannel”>
<p>パネル03です</p>
<ul class=”pannelNavi”>
<li class=”subnavi02 subnavi”>パネル02へ</li>
<li class=”subnavi01 subnavi”>パネル01へ</li>
</ul>
</li>
</ul>
▼CSS
#conts #tab #heightFix .naviWrap {
margin: 20px auto 0;
display: table;
}
#conts #tab #heightFix .navi {
-webkit-border-radius: 50px;
border-radius: 50px;
width: 100px;
height: 100px;
background: #CCC;
display: table-cell;
text-align: center;
vertical-align: middle;
cursor: pointer;
}
#conts #tab #heightFix .navi.select {
background: #AAA;
}
#conts #tab #heightFix .pannelWrap,
#conts #tab #heightFix .pannel {
margin: 20px auto 0;
padding: 40px;
-webkit-box-sizing: border-box;
-moz-box-sizing: border-box;
box-sizing: border-box;
-webkit-border-radius: 280px;
border-radius: 280px;
width: 560px;
height: 560px;
background: #efefef;
text-align: center;
position: relative;
overflow: hidden;
}
#conts #tab #heightFix .pannel {
padding: 0;
width: 500px;
height: 500px;
}
#conts #tab #heightFix .pannelNavi {
margin: 20px 0 0 -100px;
position: absolute;
bottom: 60px;
left: 50%;
}
#conts #tab #heightFix .pannelNavi .subnavi {
-webkit-box-sizing: border-box;
-moz-box-sizing: border-box;
box-sizing: border-box;
-webkit-border-radius: 10px;
border-radius: 10px;
height: 20px;
width: 100px;
background: #aaa;
display: inline-block;
cursor: pointer;
}
▼jQuery
$(function(){
//パネルの切り替え
$(‘.naviWrap .navi’).click(function(){
//クリックした要素が’.naviの中の何番目の要素かを取得
var $index = $(‘.naviWrap .navi’).index(this);
//パネルをフェードアウト
$(‘.pannel’).fadeOut();
//タブと等しいインデックスを持つpannelを表示
$(‘.pannel’).eq($index).fadeIn();
//naviからクラスを取る
$(‘.naviWrap .navi’).removeClass(‘select’);
//今クリックされているnaviにのみクラス付与
$(this).addClass(‘select’);
});
$(‘.pannelNavi .subnavi’).click(function(){
//インデックス定義
if($(this).hasClass(‘subnavi01’)){
var $index = 0;
}
else if($(this).hasClass(‘subnavi02’)){
var $index = 1;
}
else if($(this).hasClass(‘subnavi03’)){
var $index = 2;
}
$(‘.pannel’).fadeOut();
//クリックしたナビのインデックスを持つコンテンツを表示
$(‘.pannel’).eq($index).fadeIn();
//naviからクラスを取る
$(‘.naviWrap .navi’).removeClass(‘select’);
//インデックスと等しい順番のnaviにクラス付与
$(‘.naviWrap .navi’).eq($index).addClass(‘select’);
});
});
このコードが完成するまでのレビュアーAさんとコードを書いたBさんのやり取りをたどり、冗長な表現を回避するヒントを探しましょう。
冗長なコードも紹介しますので完成形のコードと比較してみてください。
HTML編：「後でやる」は100％やらない
A： まず気になったのはこのdiv. naviContainerなんだけど、これ何に使っているの？
<div class=”naviContainer”>
<ul class=”naviWrap clear”>
<li class=”navi navi01 select”>ナビ01</li>
<li class=”navi navi02″>ナビ02</li>
<li class=”navi navi03″>ナビ03</li>
</ul>
</div>
B： ……特に使ってませんね。クラスもCSSで一回も出てこない……。
A： そうであれば削除しちゃいましょう！ liタグにクラスがいっぱいついているのは理由があってのことなのかな？
B： これはタブのホバーエフェクトなどを実装する際に使用する予定です。あとプラグイン化するときにも利用する予定なので消さないでおきたいです。
A： なるほど、理由があってのクラス付けなんだね。ところでこのulについているclearっていうクラスは何をしているの？ floatの解除？
B： あ、floatを使おうと思っていたのですが、table-cellに変えたんですよね…。いらなくなったので後で消そうと思っていました。
A： ……それはすぐ消したほうがいいね。今指摘したような箇所がもう数か所あるね。そこのコードも直してすっきりさせましょう！
ポイント
コードを記述する行為の前には理由がある！
「後でやる」は100%やらない！
CSS編：書き終わったら振り返ろう！
A： いくつかのセレクタで同じマージンの指定がされているね。これ、まとめられないかな？
B： 記述が間違っているでしょうか？ 見た目は特に問題がないようなんですが……
A： 間違っている、というわけではないんだ。ただCSSをもっとすっきり見やすく、そしてメンテナンスしやすいようにしてあげたほうが楽ではないですか？
たとえば…
#conts #tab #heightFix .pannelWrap {
margin: 20px auto 0; ★
padding: 40px;
-webkit-box-sizing: border-box; ★
-moz-box-sizing: border-box; ★
box-sizing: border-box; ★
-webkit-border-radius: 280px; ★
border-radius: 280px; ★
width: 560px;
height: 560px;
background: #efefef; ★
text-align: center;
position: relative;
overflow: hidden;
}
★マークの箇所は#conts #tab #heightFix .pannelというセレクタでも同じ指定をしているよね。そうであるなら一つにまとめたほうが見た目はすっきりするね。あとどちらのborder-radiusの値も100pxに変更したくなった時に、2度直すよりは一度の修正で終わらせられたほうが楽だよね。
B： あ、それがメンテナンス性ってことなんですね！ セレクタが出てくる順番に書いていったらこのような記述になりました。一度書いたコードはもう一度見直したほうが良いのかな。
A：それも一つの手ですね。そしてまとめられるところはまとめる、消すところは消す。慣れてくると作業しながらまとめていけるようになりますよ。
ポイント
まとめられるものはまとめて記述！
jQuery編：条件分岐に要注意！
A： B君……こ、これは……・！
B： 事件ですか！？
A： ある意味事件だね……この条件分岐、どうしてこのままにしてあるの？
$(‘.pannelNavi .subnavi’).click(function(){
//インデックス定義
if($(this).hasClass(‘subnavi01’)){
var $index = 0;
$(‘.pannel’).fadeOut();
//クリックしたナビのインデックスを持つコンテンツを表示
$(‘.pannel’).eq($index).fadeIn();
//naviからクラスを取る
$(‘.naviWrap .navi’).removeClass(‘select’);
//インデックスと等しい順番のnaviにクラス付与
$(‘.naviWrap .navi’).eq($index).addClass(‘select’);
}
if($(this).hasClass(‘subnavi02’)){
var $index = 0;
$(‘.pannel’).fadeOut();
//クリックしたナビのインデックスを持つコンテンツを表示
$(‘.pannel’).eq($index).fadeIn();
//naviからクラスを取る
$(‘.naviWrap .navi’).removeClass(‘select’);
//インデックスと等しい順番のnaviにクラス付与
$(‘.naviWrap .navi’).eq($index).addClass(‘select’);
}
if($(this).hasClass(‘subnavi03’)){
var $index = 0;
$(‘.pannel’).fadeOut();
//クリックしたナビのインデックスを持つコンテンツを表示
$(‘.pannel’).eq($index).fadeIn();
//naviからクラスを取る
$(‘.naviWrap .navi’).removeClass(‘select’);
//インデックスと等しい順番のnaviにクラス付与
$(‘.naviWrap .navi’).eq($index).addClass(‘select’);
}
});
B： ……え？
A： ……長くない？
B： ……え、でもタブを動かすには必要なんです！ 3種類のパネルの出しわけをするには3種類の条件分岐が必要であって……・
A： だー！ それはわかってます！ でもこれ、長い！ 無駄に長いよ！
B： ガーン……無駄……
A： よーく見て、条件分岐の中で行っている処理、3つとも全く同じじゃない？ こういうパターンはもっと簡潔に書けるんだよ。
B： え！ そうなんですか！
A： 3つの条件分岐に対して1つの共通した処理がある。とすれば、これは条件分岐を一つにまとめて上げれば、共通した処理の記述も一回で済むんだよ。
今は3つのifが使用されているからこんなにコードが長くなってしまっているけれど……。
B： あ、else ifですか！？
A： その通り。3つのif分岐をelse ifでつなげることで、共通処理の記述が一回で済むんだ。
条件分岐は複雑になればなるほど長くなりがちです。まとめられるところはまとめて、少しでも短いコードにしたいですね。
ポイント
条件分岐で繰り返し出てくる処理はまとめられないか疑う。
A： コードレビューを終えての感想は？
B： いい勉強になりました！ 冗長にならないコードを書くポイントは、大きく分けるとたった2つのポイントに言及できると思うんです。
A： ふむふむ、ではそのポイントとは？
B： いらないものは即座に消す、まとめられるものはまとめる、です。普段の生活と同じで、整理整頓、断捨離が大事なんですね。後でとっておいたら使うかもしれない…なんてコメントアウトしておいたコードは、1時間後には忘れていますもんね……。
A： そうですね。その視点を持ち続けてタブコンテンツのプラグイン化、成し遂げてください。磨けばもっと良いコードになりますよ！
B： え、これで完成ではないんですか……。
Bさんの修業はまだまだ続きそうです。
複雑な条件分岐は条件式だけで長くなりがちなのでパッと見た時にどのような条件なのかわかるように工夫を凝らすことも必要です。どんな条件なのかコメントをつける、適度に改行をするだけでも違うものです。
こうしたブラウザ上では見えない努力は、後々Webサイトをメンテナンスするときに生きてきます。
最初に丁寧に作りこんだものは後々の作業を楽にします。
- 1630 views
- キム
