本ポストは以下の記事の日本語訳です。ご本人に、翻訳、掲載の許可をいただいています。
Code Review Best Practices - Kevin London's blog
本記事は、以下の GitHub リポジトリの gh-pages として存在しています。
https://github.com/pankona/CodeReviewBestPractices_JP_Translation
日本語訳がおかしいところや、こうしたほうが分かりやすい、等、
指摘や提案があれば ISSUE か Pull Request か、その他なんでもいいので教えてくれるととても嬉しいです。
以下、翻訳部分スタート。
Wiredrive社において, 私達はたくさんのをコードレビューしてきた。私にとっていままでそういう経験がなかったものだから、なかなか新鮮な思いである。
私がコードレビューを通して気づいたこと、良いと思っているやり方についてここで整理してみようと思う。
端的に言えば、コードレビューとは2人以上の開発者がある問題解決のために修正に対して議論することである。
たくさんの記事がコードレビューのメリット (知識の共有、コードの品質の向上、開発者のスキル向上、等) に言及している。
しかし、レビューの具体的なやり方について触れている記事はあまり存在しないと感じている。
本記事では、コードレビューにおいて何に着目するか、どのように議論するべきか、について書いていく。
単一責任の原則: 1つのクラスは1つの責務を持つという発想。メソッドにも同じ原則が適用できる。
たとえば、クラスやメソッドの説明を記載するにあたって、"and" を使わなければ説明できないときようなときは、その設計はもしかしたら適切な抽象度になっていないかもしれない。
割と単純な原則かと思うかもしれないが、確実に守っていくのはなかなか難しい。
開放/閉鎖の原則: オブジェクト指向言語を用いているのであれば、設計が拡張に対して開いており、変更に対して閉じているかを考える。
実際に機能が追加・変更されるときのことを想像して考えてみる。
（訳者補足）開放閉鎖原則についてのわかりやすそうな参考 開放閉鎖原則とexpression problem - Qiita
重複コード: "スリーストライクアウト"ルールと呼ばれる規則を適用している。
コードが一回コピーされるならしぶしぶだが甘んじて許す。だが二回目のコピーが行われたのなら、そのときは要リファクタリングである。
二回コピーが行われたということは合わせて三つの重複コードがあるので、スリーストライクでアウトである。重複部分は別途メソッドに切り出すなりしてコードの重複を防ごう。
薄目で見てみる: ぼんやりとコードの「形」を見てみる。他のコードと似通ってはいないだろうか？
他のコードと似たような「形」がたびたび登場するならば、無駄のある設計をしている可能性があるかもしれない。
来たときよりキレイに: 変更対象のコードが汚いのを目の当たりにして「うわぁなんだこれ」と思ったき、その辺りに触れたくない気持ちもわからないではないが、
良くないコードはほっといたら一生良くないコードのなままなので、ほんのちょっとずつでもマトモなコードになるようにキレイにしていくのがオススメ。
（訳者補足）ボーイスカウトルール - プログラマが知るべき97のこと
潜在的なバグの有無: 境界値周りはちゃんと処理できているか？ループが期待通り終わるようになっていて無限ループしたりしないか？
終わるときはキレイに後始末して終わるか？
エラー処理の妥当性: エラー処理はエレガントに、かつ過不足なくやれているか？
カスタマイズしたエラークラスを用いているならば、それらは本当に役に立ってるか？
効率的な設計か: アルゴリズム、データ構造は効果的な実装を選択しているか？
たとえば、配列を使うべきかハッシュテーブルを使うべきかで適切な選択をしているか、等。
メソッド名: 正しく命名するというのは、コンピューターサイエンスにおいてとても難しい問題のひとつ。
例えば、メソッド名に get_message_queue_name と付いているのに、その実、HTMLをサニタイズするような処理が行われていたりしたならば、
それは正しくないメソッド命名をしていることになるし、おそらくその関数名にダマされて誤った理解をするひとが多発してしまう。
中身を正しく表す命名をしよう。
変数名: 変数名に foo とか bar とか付けたり、例外オブジェクトに e って名前を付けたりしてしまうと、変数名に意味がなくなってしまう。
言語にもよるが、なるべく説明的な命名をするべき。変数名が意味を説明していれば、そのコードをあとで見返すことになったときに理解しやすくなるだろう。
関数の大きさ: 一関数につき、だいたい20行くらいを目安にしている。もし50行を超してしまうような場合は、処理を別の関数に切り出していくのが有効。
クラスの大きさ: クラスの大きさは100行以下が理想で、長くても300行以下にすべきだと思っている。
大きくなってしまったクラスはいくつかのオブジェクトに分割することができる場合が多く、分割して短くしてやることでそのクラスの目的がわかりやすくなる。
ファイルの大きさ: ファイルサイズが大きくなるにつれて、見通しが悪くなっていく。
Pythonのファイルならば、多くてもだいたい1000行くらいに収めるべきだと思う。それより大きくなるような場合は、別ファイルに分けて、一ファイルにさせる仕事を少なくしていくことを考える。
ドキュメンテーション: メソッドの処理が複雑だったり引数が多かったりで関数の振る舞いが曖昧なとき、それを説明するドキュメントが存在するか？
コメントアウトされたコード: コメントアウトされたコードは消してしまえ。
関数の引数の数: 関数の引数の数は3つ以下になっているか？3つより多い場合は、なんらかの方法でまとめて引数を減らせるかもしれない。
可読性: コードはわかりやすいか？頻繁に悩まなければ解読できないコードになっていないか？
カバレッジ: 新たに追加されたコードに対するテストを確認する。
テストはよく設計されているか？異常系をカバーできているか？読みやすいか？変更に強くできているか？どのぐらい大きいか？動作速度はどうか？
ちょうど良いバランスになっているか: テストをレビューするとき、バランスを見る。期待動作をチェックするのに必要十分なだけテストしているかどうか。
Gary Bernhardtさんは、95％のユニットテストと5％の結合テストというバランスを推奨している。
Djangoのプロジェクトでは、高レベルで遅いテストが作りやすく、偏りがちであるので、注意してテストを設計するようにしている。
モックの数: モックを使うのは良いが、モックが多いのは良くない。4つ以上のモックが一度に登場するならば、テスト方法について再考の余地がありそうだと考える。
そういうときは、テストが広範囲すぎるか、もしくは機能が大きすぎるかのどちらかになっていると思われる。
ユニットテストレベルでテストするのではなくて、結合テストレベルでやったほうが効率がいいかもしれない。いずれにせよ、議論するべき点である。
要求を満たしているか: レビューの終わりに、その変更がそもそもの要求を満たせているか確認する。
そうでなければ、QAにまわす前に作業しなおしてもらうべきである。
自分が変更したコードについては、変更したファイルやディレクトリに対して git add した後、 git diff --staged で、
変更内容をコミットする前にチェックするようにしている。
そのときに注意しているのは、
レビューに出す前に、まずセルフコードレビューがパスできるか確かめるるようにしたい。
他の人に指摘されるより自分に指摘されるほうが気持ちも楽だしね。:p
上述したようなコードレビューに関するよしなしごとと同様に、「レビューする人」の扱いについても多くの課題・改善の余地があることに気づいた。
より良い方法をいまだに模索中であるが、以下に実際にうまくいったやり方をいくつか紹介する。
質問してみる： どのようにこのメソッドは動作するのか？もし要求が変わったら、どこにどんな変更が必要か？
よりメンテナンス性を高めるにはどうするのがいいか？
褒める / 良いやり方を支持する： コードレビューにおける最も重要な要素のひとつは、成長・努力に対して報いることである。
同僚から賞賛されるのはかなり嬉しい。なので、私はできるだけ多くのポジティブなレビューコメントをするように心がけている。
詳細について個人的に話し込む： 設計変更などで変更が大きくなる場合は、コメントでやりとりするよりも直接話をするほうが効果的。
指摘に対する議論が行ったり来たりしてなかなか収束しないような場合も、直接話をして終わらせることがよくある。
理屈を説明する： より良いやり方がないか、なぜそう変更するのか、等の理由を説明することは、
双方 (レビュアーとレビュイー) にとってメリットがある (より建設的な議論になるよね) 。
背景の解説も説明もなしの指摘はややもすると、うっとうしく思われてしまったりするものである。
コードについての話をする： レビュー指摘するのは簡単である。特に誇りをもって業務にあたっているならなおさら。
ただし、コードについて議論をしているのであって、コードを書いた人の話で盛り上がってはならない。
議論はあくまでコードの質をあげるために行っているのであると思えば、指摘も素直に受け入れる気持ちになれる。
レビュー指摘の重要度を示す： 私はたくさんの提案を提供する傾向にあるが、それらの指摘がすべてが反映される必要はないと思っている。
指摘事項の重要さを示すことは、レビュアー、レビュイーの双方に有用な情報になる。レビュー結果が明確になり、その後の修正作業もしやすくなる。
開発者は、動作するのはもちろんのこと、加えてメンテナンスしやすいコードを書く責任がある。
ただ、納期におされてメンテナンス性がおざなりになることがしばしばある。
リファクタリングは外部的な振る舞いや機能を変えない、内部的な変更である。
一見すると地味でつまらなく思うかもしれないが、その価値を低く感じる必要はまったくない。
リファクタリングによるメンテナンス性の向上は、バグ修正と同じくらい重要であると心得よ。
ややもすればレビュー指摘は個人攻撃のように思えてしまい、レビューに対して臆病な気持ちになることもある。
しかし、コードレビューはあくまでコードレビュー。なるべくオープンな気持ちを持ち、指摘は真摯に受け止めよう。
レビュアーの指摘に対して、特に反論がないとき、指摘通りに修正してよいだろう。
レビュアーが変更について質問をしてきたとして、仮に説明できないとすると、
それは理屈のない、なんとなく書いたコードということを意味しており、つまり、将来混乱をまねく部分になるかもしれない。
また、コードへの変更は、大きな設計上の欠陥やバグを発見するキッカケになる可能性もある。
通常、指摘は行単位につけられている。
レビューには Stash を使っているが、私はレビュー指摘を見つけると同時に、修正コードをコミットするようにしている。
そうしないと、どの件に対応してようとしていたか、忘れてしまうことがあるので。
キレイなコードを書く技術に関する書籍はたくさん存在する。
いくつか紹介する。
良さ気な動画もある。
Tweet
Copyright © 2015 pankona