ソースコードレビュー時のチェック観点
- 一般
- 1つの話題（API追加・仕様変更・バグ修正etc）につき1つのIssueを立て、コミットログにIssue IDを書いているか（メジャーバージョンアップを除く）
- 関数/変数の名前は適切（綴り・長さ・意味・対称性・既存APIとの統一性 etc）か
- 変数宣言のvarを忘れていないか。var a = b = c = 0; のように書いていないか(書いたら×)
- forループのlengthはキャッシュしているか
- 1関数内で頻繁に参照する深いプロパティアクセス(x.y.zのようなアクセス)は適切にキャッシュしているか
- モジュール内でprivateで使用する関数(変数)は適切な場所に書かれているか
- 引数チェック・nullチェック
- デバッグコードが残っていないか（console.log(), alert()）
- API設計・実装
- テスト記述
- 環境・ブラウザ
- jQuery
- イベント処理
- 非同期処理
- ドキュメンテーション
一般
1つの話題（API追加・仕様変更・バグ修正etc）につき1つのIssueを立て、コミットログにIssue IDを書いているか（メジャーバージョンアップを除く）
関数/変数の名前は適切（綴り・長さ・意味・対称性・既存APIとの統一性 etc）か
公開APIの名前には特に気を付ける。
原則として、関数は動詞、名詞形にする
変数宣言のvarを忘れていないか。var a = b = c = 0; のように書いていないか(書いたら×)
- varを忘れたらグローバルになってしまう
- a = b = c = 0;だとb, cはグローバルになってしまう
- QUnitのグローバル化チェックで検出する
forループのlengthはキャッシュしているか
JavaScriptではlengthを直接見ると毎回プロパティアクセスになって遅くなる。
特別な事情がない限り、for(var i = 0, len = array.length ; i < len ; i++) のように初期化式中でキャッシュする。
1関数内で頻繁に参照する深いプロパティアクセス(x.y.zのようなアクセス)は適切にキャッシュしているか
深いプロパティを毎回たどると遅くなる。
キャッシュ可能ならキャッシュする。
モジュール内でprivateで使用する関数(変数)は適切な場所に書かれているか
先頭のFunctions(Variables)の箇所に記述する
引数チェック・nullチェック
valueに空文字列や数値の0が来る場合に if(!value) を使っていないか
暗黙型変換の結果、意図せず分岐を通ってしまう可能性がある。
デバッグコードが残っていないか（console.log(), alert()）
API設計・実装
新規API追加時
関数名・変数名・仮引数名・パッケージ位置は適切か
既存のAPIと矛盾しないか
@sinceタグをつけているか（記述例：@since 1.1.0）
JSDocを記述しているか（公開APIに対してはJSDoc必須）
仕様変更時
JSDocを書き換えたか
既存のリファレンス・チュートリアルと整合は取れているか
内部実装変更時
現行の仕様通りに動作しているか
- 内部実装のみ変更する場合、仕様は変更しない（当然）
- 基本的には、現行のテストケースがすべて通るかどうかでチェック
必要に応じてテストケースを追加しているか（基本的にはリファクタでケースは減らさない）
- 場合によっては今まで気づいていなかったケースが見つかることもある
- カバレッジが100%でならなくなってしまう場合もある
- その場合、多くは（大小はあれど）何らかの仕様変更を伴っていると思われるが
その他・一般
プライベートプロパティは先頭に"_"をつけているか
（特に公開APIについて）引数チェックは塩梅を見極める（コードサイズ増大とのトレードオフ）
基本的には事前条件チェックはきっちり行い、問題があれば詳細なエラーを出すべき。
ただし、同時にそれはエラーチェックのコードが増えていくことを意味する。
JSファイルはクライアントに送られるので、コードサイズの増大には慎重になる必要がある。
簡単なコードでチェックできるような場合は行うべきと考えるが、
通常来ないであろうパターンのチェックなどは省略してよいとする。
テスト記述
ブラウザの画面サイズに依存したテストコードになっていないか
テストが実行される際のブラウザサイズは不定。
よって、「サイズが1000pxあるはず」などを仮定してはいけない。
また、「ブラウザは最大でも1900px以上の幅にはならないはず」のような仮定も置かない。
innerWidth()など、サイズを取得してその値に基づいてテストされるようにする。
テストケースの粒度は適切か
1つのテストケースでは、基本的に1つの機能/側面について確認する。
1ケースで様々なことを確認ようにしていると、仕様変更やバグ修正の際
テストが失敗した時何が原因で失敗しているのかわかりづらくなり、
ケースの修正が必要な場合修正にも時間を要する。
環境・ブラウザ
ブラウザ間の互換性をとるための分岐判定として、可能な限り機能判定を行っているか
- UserAgentによる判定は可能な限り避ける
*特にIEの場合、ブラウザモード×ドキュメントモードの組み合わせがあるので、UA判定を用いると動作検証が大変- 「IE9ベースだけどIE7のUA文字列」のようなことをされると、判定に失敗する恐れも
IE上の動作をテストするとき、ドキュメントモード・ブラウザモードを変えてテストしたか
例えばUserAgentを使用して処理を分岐させている場合、
ドキュメントモードだけでなくブラウザモードが関係してくる。
- ドキュメントモード
- CSSの挙動、サポートするHTML5 API、JavaScript APIなどの動作が変わる
- ブラウザモード
- User-Agentを変更する
参考：Internet Explorer 8 の開発者ツールの概要
イントラネット設定に依存していないか
IEでは、イントラネット扱いされたドメインのページが自動的に互換表示になってしまうことがある。
検証用ページでは、必要に応じてX-UA-Compatibleを使ってIEのドキュメントモードを固定しているか
固定しておかないと、同じ状態のテストにならない恐れがある。
jQuery
「クエリの結果マッチした要素がない」ことのチェックは!$()でなくlength等で行っているか
jQueryでクエリした場合、マッチした要素がなくても、戻り値はnullではなくjQueryオブジェクトである。
従って、「要素がないこと」のチェックをnullチェックで行ってはいけない。
XML/VML/SVGの処理にjQueryを（直接）使ってしまっていないか
jQueryは、XML/VML/SVGに対する操作は対応していない（ノード操作の場合正しく動く場合も多いが、きちんとケアされているわけではない。）
参考：http://docs.jquery.com/Won't_Fix#SVG.2FXML.2FVML_Bugs
イベント処理
ハンドラを登録したら、適切に削除しているか
イベントハンドラを登録したままにしていると、メモリリーク、処理が多重に呼ばれる（パフォーマンス劣化）などの影響がある。
特にコントローラの処理でハンドラを独自に登録した場合、それらはdisposeされても
自動アンバインドされないので注意が必要。
ハンドラ内のthisは通常、イベント発火元であることを考慮しているか
通常、イベントハンドラが呼ばれたときthisはイベントを送出したオブジェクトになっている。
obj.myHandler = f(){} があったとき、 eventDispatcher.addEventListener('event', obj.myHandler) をすると
myHandlerのthisはeventDispatcherになっているのが普通。
勿論、考慮する必要がなければ何もしなくてよい。
非同期処理
イベントハンドラ内でタイマー（setTimeout, setInterval）を使っている場合、タイマー完了前にイベントが複数回起きる可能性を考慮しているか
「イベントAが起きたらt秒後に特定の処理を行う」ようなコードを書いている場合、その処理が何度も起きてしまう可能性がある。
Aが起きたら「1回」動けばいいだけ、の場合は、タイマーIDを適切に保存しておき
完了前にイベントが起きたらclear -> setし直すようにする。
ドキュメンテーション
ソースコードコメントを適量書いているか。「なぜこういう処理をしているか」が書いてあるか。
特に、ブラウザ依存の問題を回避するための分岐、別のAPIで行われる特殊な前提条件に基づいた動作をするコードなどは
十分にコメントを書く。