Diverse Advent Calendar の18日目です。
今までまともにコードレビューをしたことがなかったエンジニアが、半年前からレビューを経験し始めて、チームの実力を底上げした（と思っている）レビューの仕方をご紹介します。
ここで説明すること、しないこと
ここでは以下のことについて説明します。
- master のコードの質を上げるためのチェックポイント
- 自分のレベルを上げるコードリーディングの方法
以下のことは説明しません。昨日の mixi Advent Calendar で記事にしましたのでそちらをご参照ください。
- コードレビューの雰囲気を良くする（良く保つ）方法
- 自分やチームメンバーの成長を促すための心がけ
なぜ見るべきところとそうでないところがあるのか
効率的にレビューを進める必要があるためです。
レビューに時間をかけすぎてもあまり良いことがないのです。
レビューワーの仕事は止まるし、レビューイーもコメント待ちの間に別の仕事をしようにもコンテキストスイッチが発生することになります。
手早く（エンジニア職の楽しいところである）コードの設計と記述に戻りたい、という気持ちは誰しも抱えていると思います。
ですが、レビューをないがしろにする訳にはいきません。
master （など主要ブランチ）への明らかなバグの混入を防いだり、コードのメンテナビリティを維持・向上させる必要があるためです。
レビューを的確に効率よく進めるため、見るべき箇所を絞っておく、というのは一つの手段として有効なのではないでしょうか。
こうしたチェックポイントを知っておくことで、以下のことが期待できます。
- レビューワー（レビューする人）はレビューの負担を減らすことができます
- レビューイー（レビューを受ける人）は完成度の高いコードを書くことができ、修正にかける時間を減らすことができます
チェックポイント
プロジェクトの構成・作業フローによってチェックポイントは異なりますが、ここでは以下を想定しています。
- master へのマージをレビューする
- コードスタイルが決まっており、 Lint ツールで確認可能
- 自動テストあり
- 一部の単体テストのみ
- 結合テストまでは自動で行わない
- レビュー提出前に自分で動作チェックを行う、という決まりはある
- レビューワーも手元で動作チェックする
- ドキュメントはドキュメントコメントから自動生成
また、チェックポイントをレビューして修正が必要であれば当然指摘すべきですが、指摘の必要がないと思ったら
「この書き方丁寧で良いですね！！ 勉強になります！」
など、前向きなコメントをすると良いかと思います。
実際に私も、レビューワーとしてレビューしている中で、知らない文法を学ばせていただいたり、気を付けた方が良いことを教えていただきました。
まずはあまり時間のかからないところから。
動くかどうか
期待通りに動作するかどうか、異常値を入れたらどうか、など、簡単な動作チェックをする必要があります。
これは人間の手で行う必要はありません。
自動テストを行う環境が整っていれば、テストの結果で判断できます。
（自動テストで判断するなら、テストコードが妥当であるかは確認する必要があります）
コードスタイルに則っているかどうかも一緒に確認すると良さそうです。
Lint ツール走らせれば一発でわかりますし。
誤字脱字はないか
自分には何のことだかわかるような軽いスペルミスでも、後から他人が見れば何のことだかわからずに混乱させてしまう元になることがあります。
エディタにスペルチェッカなどがあればそれで検査するようにできます。
が、例えばローマ字を使う場合など、スペルチェッカを適用できない箇所については人間がチェックすることになると思います。
（都道府県を定数で宣言するコードを書く時にレビューで書き間違いを指摘していただいて助かったことがあります）
ドキュメントコメントは書かれているか、書かれていることは正しいか
ドキュメントコメントの有無のチェックくらいは Lint ツールなどに任せても良さそうです。
現代のプログラムでは書かれていることの妥当性をカジュアルに確認できるツールがないため、内容が正しいかどうか、不足はないかは人間がチェックしましょう。
具体的に何を書くべきか、ということについては maku77 さんの記事が詳しいので、そちらを参照していただくと良いと思います。
Javadoc と銘打っていますが、書かれているのは本質的なことです。
// Don't
/**
* 取得
*/
public HogeHoge fetchHogeHoge(int fuga) { ...
// Do
// 自分以外の使う人のことを考えて書きましょう
/**
* HogeHoge をサーバから取得します。
* このメソッドはスレッドをブロックするので使用時は注意してください。
* @param fuga HogeHoge の fuga 値。 0以上である必要があります。
*/
public HogeHoge fetchHogeHoge(int fuga) { ...
命名は実態に即しているか、名前だけで意味がわかるか
パッケージ名、クラス名、メソッド名、メンバ名などがチェック対象になります。
余裕があれば、メソッドの引数もチェックしても良いかもしれません。
ローカル変数については特段チェックする必要はなさそうです。
名前がわかりやすいか、そうでないかによって、メンテナビリティが大きく変わってきます。
例えば戻り値が void 型のメソッドなのに get〜〜 という接頭辞が付いていたらどう思いますか？
public void getMember() { ...
私だったら頭を抱えます。どこから Member を取ってきて、そしてどうしてくれるつもりなんでしょう…
クラスやメソッドの振る舞い、戻り値やメンバのデータ型によって、または使用する言語によって適切な（慣例的な）命名があります。
後継者に恨まれないようにするためにも、適切な名前のものを master に残しましょう。
命名の仕方についてはたくさんの方が提案をされていますので、どれかを参考にすると良いのではないでしょうか。
いいとこ取りをして、チーム内の命名規則を作るのも良いかもしれません。
テスト可能な粒度・設計か
特にクラスやメソッドが過剰に大きくなったりしていないかを確認すべきでしょう。
すべての Fat Controller を生まれる前に消し去りたい
また、テスト可能な設計にするためには、なるべく状態を持たない（あるいは初期状態を外部から設定しやすい）ようにする、など気をつけるべきことが多くあります。
テストしやすい設計については @t_wada さんが CodeIQ にかかれた解説がわかりやすいです。
（Android 開発に固有の問題として、 JVM 上で単体テストできる設計になっているかどうか、という問題もあるかと思います。 TDD するのにいちいち Robolectric 立ち上げるの辛くありませんか…？）
設計は妥当か
Github の Pull Request や Gitlab の Merge Request の diff 画面だけ見ているとつい忘れがちになりますが、 はたしてこの変更はこの場所に入れて良いのか 検討することは重要です。
- クラスの役割が多すぎないか、単一責任原則を守っているか
- 配置するパッケージは間違っていないか
- などなど
また Clean Architecture のようにデータや処理の方向を制限する設計を採用しているプロジェクトの場合は、許可されていない依存関係が生じていないかも確認が必要でしょう。
妥当な設計、特に単一責任原則については、以下の記事がわかりやすかったです。
どのチェックポイントを採用するか
プロジェクトがどの程度レビューに時間を割けるかによるのではないでしょうか。
レビューに時間を割くことができるならなるべく多くチェックすることができますし、
時間がないならドキュメントコメントの有無とメソッド名のチェックだけ、というようにすることも可能です。
また、レビューワーの立場にもよるかと思います。
コードの品質に責任を持つ立場なら、設計レベルで妥当性を確認することになると思います。
以上、明日からコードレビューが早くなる、チェックすべきポイントでした。
明日はきっと誰かが面白いことを書いてくれると思います。