Wiredrive では、私たちはかなりの数のコードレビューを行います。しかし、ここで働き始める前には私はコードレビューなどしたことがありませんでした。今回は、私がコードレビューをする時に何に注目するようにしているかや、私の考え出したベストなコードレビューのやり方をお話したいと思います。

コードレビューとは、簡単に言うと2人以上の開発者で問題を引き起こしそうなコードの修正について話し合うことです。コードレビューをすることのメリットについては多くの記事で語られており、知識を共有できること、コードのクオリティが上がること、開発者が成長できることなどが挙げられています。しかし、レビューを行う上で、どのように進めていくかという具体的なことについてはあまり多く語られてないように私は思いました。

レビューで何に注目するか

アーキテクチャ/デザイン

• 単一責任原則 : 1つのクラスは変更する理由が2つ以上あってはいけないという考え方です。これは意外と大変なことだと思います。私はメソッドにもいつもこの原則を採用しています。1つのメソッドが行う処理を説明する際にandと言わなければいけない場合は、抽象化のレベルが適切でないのかもしれません。

• 開放/閉鎖原則 : 言語がオブジェクト指向の場合、そのオブジェクトは拡張に対して開いていて（拡張が可能で）、修正に対して閉じている（他のオブジェクトに影響を与えない）でしょうか？ もしxをもう1つ追加したらどうなりますか？

• コードの重複 : 私は “三振法” を採用しています。私は好みませんが、コードが一度コピーされるのは大抵問題ありません。さらにもう一度コピーされるのであれば、共通の機能を分割するにするようにリファクタリングするべきです。

• 目を細めてコードをチェック : 目を細めてぼやけた視点からコードを見た時、そのコードは他と形が同じでしょうか？　コードの構造上、別の問題が起こりうるパターンはないでしょうか？

• 見つけたときよりもマシなコードにする : グチャグチャになったコードの周辺を修正するというのは、いくつかの行で追加をしていくということです。その際、より高みを目指して、該当箇所を見つけたときよりも良いコードに修正する意識を持ちましょう。

• バグの可能性 : Off-by-oneエラーはありませんか？　ループは想定していたとおりに止まりますか？　それは本当に止まりますか？

• エラーの処理 : エラーは必要な場所で丁寧に明確に対処されていますか？独自なエラーを追加してはいませんか？もしその場合、それは本当に有用なものですか？

• 効率 : コードの中にアルゴリズムが入っている場合、それは効率的な実装といえますか？　例えば、リスト中のキーについて、ディクショナリの中で何度も繰り返されているものがあったら、それは求める値を探すのに非効率です。

スタイル

• メソッド名 : コンピュータ･サイエンスの中でも名前をつけるというのは難しい問題の一つです。メソッドが get_message_queue_name という名前で、インプットとは全く違ってHTMLをサニタイズするような動作をしているとしたら、それは適切ではないメソッド名です。誤解を招く機能となってしまうでしょう。

• 変数名 : foo か bar はおそらくデータ構造では不便な名前でしょう。 exception と比べれば、 e も同じように不便でしょう。言語にもよりますが、名前は必要十分な冗長さにしましょう。変数名がうまく表現されていれば、あとで再度見直したときに、簡単にコードを理解することができます。

• 関数の長さ : 関数の長さは大まかに20行かそれ以下であるべきだと私は考えます。50行以上のメソッドは、短く分けるべきです。

• クラスの長さ : クラスは多くても計300行以下、理想は100行以下であるべきです。巨大なクラスは大抵の場合、別のオブジェクトに振り分けることができます。クラスの本来の役割を考えましょう。

• ファイルの長さ : Pythonのファイルの場合、1つのファイルに入れるのは長くても1,000行程度までにすべきだと思います。それ以上長くなっているということは、もっと特化した小さなファイルに分割すべきだというサインです。ファイルのサイズが大きくなればなるほど、何かを探す時に見つけづらくなります。

• docstring : 複雑なメソッドや長い引数リストを持つメソッドについて、それぞれの引数が何をするのかが明確でない場合、それを説明するdocstringはあるでしょうか？

• コメント化されたコード : コメントアウトされた行は取り除いてしまった方がよいでしょう。

• メソッド引数の数 : メソッドや関数の持つ引数が3つ以下になっていますか？　3つよりも多い場合は、別のやり方でグループ化した方がよいかもしれないというサインです。

• 可読性 : コードは分かりやすいですか？　レビューの際、何度も止まってしまうことはありませんか？

テスト

• テストカバレッジ : 私は新しい機能に対するテストを見るのが好きです。そのテストはよく考えて作られているでしょうか？　不具合の条件をカバーできているでしょうか？　読みやすいですか？　脆弱性はどの程度でしょうか？　テストの規模は？　遅くないですか？

• 適切なテストレベル : 私がテストのレビューをする時は、適切なレベルでテストが行われているかを確認します。つまり、期待する機能性をチェックするのに必要な下位のテストがきちんとできているかを確認するのです。Gary Bernhardは、95%の単体テストと5%の結合テストという割合を推奨しています。特にDjangoのテストでは、偶然にも上位のテストが簡単にできて低速なテストスイートを作成してしまうことがありますので、慎重になることが大切です。

• モックの数 : モックは素晴らしいものですが、全てをモックで代用するのはよくありません。私は自身の経験から、1つのテストに3つ以上モックがある場合は再考が必要だと判断しています。こういう時はテストの範囲が広すぎるのか、その関数が大きすぎるのかどちらかでしょう。もしかしたら単体テストのレベルでテストを行う必要はなく、結合テストで十分なのかもしれません。どちらにしろ、議論の余地があるということです。

• 要件を満たすこと : 通常、私はレビューの最後に、ストーリーやタスク、報告されているバグについての要件に注目します。もしその基準を満たしていないようであれば、QAに進む前にやり直した方がよいでしょう。

最初に自分の書いたコードを見直す

コードを提出する前に、私はよく更新対象のファイルやディレクトリに対して git add を行い、続いて git diff --staged を実行します。まだコミットしていない変更について調べるためです。通常、以下のようなことをチェックします。

• コメントやTODOが残っていないか。
• 変数名が的確か。
• その他、ここまでに挙げたこと。

私は人にレビューしてもらう前に、自分でコードレビューを行い問題がないことを確認したいのです。人から指摘されるより自分で気づいた方が気持ち的にも楽ですからね。

コードレビューのやり方

コードレビューの中でも人間が行う部分は、毎回全てが試練だと感じます。このパートをどのように行うのがよいのか、私も未だ勉強中です。これまでコードについて議論する際にうまくいったアプローチを以下にご紹介します。

• 質問する : このメソッドはどのように機能するのか、仕様が変更になった場合は他にどこを変更する必要があるのか、もっと保守しやすくするにはどうすればよいか、などを質問します。

• 褒める、グッドプラクティスを強化する : コードレビューにおいて最も重要なことの1つは開発者の成長や努力に報いることです。同僚からの賞賛以上に嬉しいことなど、あまりないでしょう。私はできるだけたくさんポジティブなコメントをするよう心がけています。

• より細かいポイントについて直接議論してみる : 時には、推奨されたアーキテクチャの変更が大きいがために、コメントでやりとりするより直接会って議論したほうがが簡単な場合があります。同じように、ある点についての議論が前後してしまう場合、私はポイントをピックアップして、議論を終えてしまうこともあります。

• 理由を説明する : もっとよい選択肢が他にあるかどうか尋ねたり、なぜ修正するべきかを明確にしたりすることは、ベストなことだと思います。時々、コンテキストや説明なしでは、提案された変更があら捜しのように感じられてしまうからです。

• 指摘はコードに対して行う : レビューから個人的に気づいたことをメモするのは簡単なことです。自分の仕事にプライドをもっていれば、なおさらです。思うに、開発者についてではなく、コードについて検討することが大事です。抵抗感もなくなりますし、開発者うんぬんではなく、コードのクオリティの改善が必要だからです。

• 修正の重要性を提案する : 私はたくさんの提案をしがちですが、全てが反映されるべきとは限りません。修正に取りかかる前に、修正が必要かどうかを明確にすることは、レビューする側にとっても、される側にとっても有効です。それにより、レビューの結果を、明瞭かつ即実行可能なものとします。

考え方について

開発者として、私たちはコードの動作とメンテナンス性の両方に責任があります。動くコードを納品するための圧力によって、メンテナンス性が後回しになってしまうことは起こりがちです。リファクタリングは機能を設計レベルで変えてしまうことはありません。ですから、勧められた変更にがっかりする必要はないのです。コードのメンテナンス性を高めることは、バグを引き起こしているコードを修正するのと同じぐらい重要です。

さらに、コードのレビューを行っている間は、広い視野を持つようにしてください。これこそが、誰もが悪戦苦闘するところなのです。誰かに自分の書いたコードをもっとよくすることができると指摘された時、個人的に批判されたと感じ、身構えてしまうことがあるからです。

もしレビューする人が提案してきた時に、その提案を否定する理由が自分ではっきりと分からない場合、大抵はその提案通りに修正を実行します。レビューした人がコードの一部について質問してきた場合、それは今後、他の人にも混乱を引き起こす可能性があるでしょう。加えて、変更を行うことはより広い範囲でのアーキテクチャの問題やバグを明らかにしてくれることもあります。

（このセクションを付け加えるよう勧めてくれたZach Schiponoに感謝します）

提案された変更に対処する

私たちは大抵の場合、コードの背景について考えながら、行単位ベースでコメントを残しています。通常、私たちはStashでレビューを見ますが、同時に提案された変更を実行するためにコードをpullします。すぐさま処理をしないと、対処すべきはずのものが何だったのか忘れてしまうことがあります。

その他の参考資料

明瞭なコードを書くための技術について書かれた本は多くあります。読みたい本は数あれど、通して読めた本はそのうちのほんの僅かです（それを変えるため頑張っていますが）。以下がそのリストです。

• Clean Code
• Refactoring

私は講演の動画が大好きです。この記事を書く間に参考にした、 ちょっとした役に立つ、関連した講演 を以下にリンクしておきます。

• Sandi Metzによる全ての小さなこと : 全体的に問題を網羅していて、とりわけクリーンで再利用可能なコードを書くという視点でうまくまとめられています。

• 優れたAPIの設計方法とその重要性について : ここで言うAPIは、アプリケーションの構築方法と、またそれが、どのように私たちとインタラクションするかを意味します。映像ではこの記事で取り上げたことと類似したテーマが多く出てきます。