BT

Your opinion matters! あなたのご意見でInfoQが変わる!

コードレビューをより効果的にする方法

| 作者 Trisha Gee フォローする 0 人のフォロワー , 翻訳者 笠原 王徳 フォローする 0 人のフォロワー 投稿日 2016年11月18日. 推定読書時間: 21 分 |

A note to our readers: As per your request we have developed a set of features that allow you to reduce the noise, while not losing sight of anything that is important. Get email and web notifications by choosing the topics you are interested in.

原文(投稿日:2016/10/01)へのリンク

重要なポイント

  • コードレビューを行うとき、本当に多くの着眼点がある。チームは何がこのプロジェクトで大切なのかをきちんと特定し、レビューの間に一貫してこれらの確認を行わなければならない。
  • 人間によるレビューはとても貴重なので、できることは全て自動化する必要がある。例えば、フォーマット、スタイル、よくあるバグに対する確認、よくあるのセキュリティ課題の特定、自動化されたテストの実行など。
  • パフォーマンスに話題が及ぶとき、システムのパフォーマンス要件を理解することはどれが潜在的な問題であるかをコードレビューの間に探すための鍵となる。
  • 人間のコードレビュアーによるいくつかの単純な確認を行うことで、アプリケーションのセキュリティを著しく改善することができる。
  • コードレビューは協調的な議論であるべきで、格闘ではない。何が大切で、何がそうでないかを前もって決定しておくことで、摩擦を軽減し期待を管理することができる。

 

チームでコードレビューを行うことが、コードの品質を向上させること、知識と責任を共有すること、よりよいソフトウェアとチームを構築することに寄与することは広く知られている。 もしコードレビューに関して情報を検索する時間を少し取ってみれば、コードレビューがもたらす価値に関する記事をたくさん見つけることができるだろう。レビューを行う方法もたくさんある。GitHubでプルリクエストを行う、JetBrainsのUpsourceのようなツールを使用する、など。しかし、やり方が明確で正しいツールを用意した場合でも、大きな疑問が残る。何をレビューで探せばよいのだろうか。

おそらく何を探せばよいかに関する決定的な記事がない理由は、検討すべき事柄が多種多様に渡るためである。そして、他の全ての要件と同じく、個々のチームが置かれた状況によって異なる優先度を持つことになるためである。

この記事では、レビュアーが探すことができるいくつかの事柄について概略を説明することを目的とする。それぞれの状況で優先度を決定する行為はプロジェクトの特性に依存する。

ツールに任せることができないどんなことを人間は指摘できるのだろうか?

驚くほど多数の事柄があることがわかっている。この記事の残りで幅広い重要事項のリストに触れ、2つの特定の領域、パフォーマンスとセキュリティに関してはもう少し深く言及する。

設計

  • 新しいコードは全体アーキテクチャに適合しているだろうか?
  • コードはSOLID原則、ドメイン駆動設計、もしくはチームが採用する他の設計手法に従っているだろうか?
  • 新しいコードでデザインパターンは使用されているだろうか?これらは適切だろうか?
  • コードベースの標準や設計スタイルが混合されている場合は、新しいコードは現在の原則に従っているだろうか?コードは現在の方向性を引き継いでいるか、徐々に除去される古いコードの例に従っているだろうか?
  • コードは正しい場所に配置されているだろうか?例えば、コードが注文に関係する場合は、それは注文サービスの中にあるだろうか?
  • 新しいコードは既存のコードのどこかを流用することはできないだろうか?新しいコードは既存のコードで流用可能な何かを提供できないだろうか?新しいコードはコードの複製を招いていないだろうか?もしそうなら、より再利用可能なパターンにリファクタリングべきだろうか?それとも現在の段階ではそれは受け入れられるものだろうか?
  • コードはオーバエンジニアリングされていないだろうか?現在の必要とされていない再利用性により構築されていないだろうか?再利用性とYAGNIの間でバランスをとる配慮がチームでされているだろうか?

可読性とメンテナンス性

  • (フィールド、変数、パラメータ、メソッドとクラスの)名前はこれらが表現する事柄をきちんと反映しているだろうか?
  • コードを読むことによって、何をしているのか理解することができるだろうか?
  • テストが何をしているのか理解することが出来るだろうか?
  • テストはの実際の事例の良いサブセットを網羅しているだろうか?これらは正常系と例外系を網羅しているだろうか?考慮されていない事例があるだろうか?
  • 例外時のエラーメッセージは理解可能か?
  • 混乱しがちな部分が(チームの選択により)文書になっていたり、コードにコメントがされていたり、理解可能なテストに落とし込まれているだろうか?

機能性

  • コードは行うと想定されることを本当に行なっているだろうか?コードの正確性を保証するための自動テストが存在し、テストは承認された要件を満足するコードを本当にテストしているだろうか?
  • ちょっとしたバグがコードに含まれていると思われるだろうか?例えば、確認のための誤った変数を使用する、うっかり本物と代替のものを併用したりするなど。

こんなことを考えたことがあるだろうか?

  • 適合することが必要な規制要件はあるか?
  • 作者が公開文書を作成したり、きぞんのヘルプファイルを変更する必要はあるか?
  • ユーザが遭遇するメッセージが正確性を記すように確認されただろうか?
  • 製品において処理を止める明確なエラーはあるだろうか?コードがうっかりテストデータベースを指すことになったり、本番のサービスに取り替えられるべきハードコードされたスタブが存在したりするだろうか?
  • 性能に対する要件はなんだろうか?セキュリティへの影響を検討したことがあるだろうか?これらは広範な領域であり、鍵となる話題であるため、以下でこれらの両方に関してより詳細を取り扱うことにする。

性能

性能に関する話題の詳細に入っていこう。ここはコードレビューから利益が本当に得られる領域である。

全てのアーキテクチャや設計において、システムの性能に関する非機能要件は前もって設定されるべきである。ナノ秒単位で反応する必要のある低遅延取引システム、もしくは"To Do"を管理するモバイルアプリケーションを開発したことがあれば、何をもって"遅すぎる"とするのか想像がつくだろう。

性能に関するコードレビューを着手するかどうかを決める前に、要件が何かに関して自問するべきだ。ある種のアプリケーションにおいてはミリ秒毎に何をしているかを検討することが必要であるが、大部分のアプリケーションはほんの数サイクルを改善する最適化に何時間も費やされており、これはあまり価値をもたらさない。しかし、よくある避けるべき性能上の落とし穴に落ち込まないよう保証するために、レビュアーが確認することができる点がある。

この機能には厳しい性能要件が存在するだろうか?

このレビューされるコードは以前公開されたSLAに関係する領域内だろうか?もしくは、必要とされる性能特性を要件内で記述していることはあるだろうか?

もしコードが"ログイン画面が読み込まれるのが遅い"といった課題に対するものであれば、オオリジナルの開発者が適切な読み込み時間を決定する必要がある。そうでなければ、どうレビュアーや作者が速度が十分に改善したと自信を持てるだろうか?

もし厳しい要件があるのであれば、これらに適合していることを証明するテストはあるだろうか?性能がクリティカルなシステムにはどれも自動化された性能テストがあり、公開されたSLA(全ての注文要求は10ms以内にサービスされる、など)を満足することを保証しなければならない。これらがなければ、SLAの達成に失敗したことをユーザの報告により知ることになる。これはお粗末なユーザ体験であるというだけでなく、避けられる罰金や課金を支払うことになる恐れがある。

修正もしくは新規の機能性は 、既存の性能テストのいずれかの結果を劣化させる影響を及ぼすだろうか?

もし定期的に性能テストを実行する(もしくは必要に応じて実行可能なテストスイートを持っている)のであれば、性能がクリティカルとなる領域でシステム性能の劣化を招いていないことを確認しよう。これは自動で処理をできるかも知れないが、CI環境で性能テストを実行することは単体テストの実行よりも一般的ではないため、レビューの間に実行可能なステップとして特別に言及する価値がある。

サービス/アプリケーションの外部からの呼び出しはコストがかかる

ネットワークのホップが必要なアプリケーション外部からのシステムの利用は、一般的にどんなものでも貧弱な最適化を超えるコストがかかる。以下のケースを考えてみよう。

  • データベースの利用:最悪の問題児はORMのような抽象化の背後に隠れているかも知れない。しかし、コードレビューではループ内での都度データベースを呼び出すような、性能問題でよくある原因を発見できるようになるべきである。例えば、IDのリストを読み込み、その後そのIDに対応する個々の項目毎にデータベースクエリを行うというようなものである。
  • 不要なネットワークの利用:データベースと同じように、リモートのサービスは時に濫用される。つまり、一回の呼び出しで十分であったり、バッチやキャッシュによりコストのかかるネットワーク呼び出しを防止できる可能性がある場面で複数のリモート呼び出しが行われる。繰り返すと、データベースのように、時々抽象化によりメソッドがリモートAPIを呼び出していることを隠蔽することに留意すること。
  • モバイル/ウェアラブルアプリケーションがバックエンドの呼び出しを過剰に行う:これは基本的には"不要なネットワークの利用"と同様だが、モバイル端末における追加の課題は、バックエンドの不要な呼び出しは性能を劣化させるだけでなく、バッテリーの消費を早め、ユーザに金銭的消費を強いることになる点である。

リソースの効率的・効果的な使用

コードは共有リソースへのアクセスのためにロックを使用しているだろうか?その結果、性能劣化やデッドロックを招いていないだろうか?

ロックは性能を低下させる要因であり、マルチスレッド環境で原因を推測するのがとても困難である。以下のようなパターンを検討してみよう。他の全てのスレッドが自由にリードできる中で、1つのスレッドだけが値を書き込み・変更できるようにすること、もしくはロックフリーのアルゴリズムを使用すること。

  • メモリリークはあり得るだろうか?Javaの場合、よくある原因は、変更可能なstaticフィールド、ThreadLocalの使用、もしくはClassLoaderの使用。
  • メモリの使用量が無限に増加しないか?これはメモリリークと同じではない。メモリリークはガーベジコレクターにより収集できない未使用なオブジェクトが存在する場合である。しかし、ガーベジコレクトをしないものも含め、どんな言語でも無制限に成長するデータ構造を生成することができる。もし、レビュアーとして新しい値が定期的にlistやmapに追加されているのを発見したら、そのlistもしくはmapが破棄・削減されるかどうか、されるのであればいつ行われるかを質問してみよう。
  • コードは接続やストリームを閉じているだろうか?接続、ファイルやネットワークストリームの閉じ忘れは容易に発生する。別の誰かが書いたコードをレビューしている時、もしファイル・ネットワークもしくはデータベース接続を使用していれば、正しく閉じられているようにしよう。
  • リソースプールは正しく設定されているか?ある環境に対して最適な設定はいくつもの因子に依存しているため、例えばデータベースの接続プールのサイズが正しく設定されているかということは、レビュアーとしてすぐにわかるというのは考えづらい。しかし、いくつかのことは一瞥して伝えることができるだろう。例えば、プールのサイズが明らかに小さい(例えば1つ)であったり、大きすぎる(100万オーダのスレッド)ということである。もし疑わしいのであれば、デフォルト値は通常良い出発点である。デフォルト値から逸脱しているコードはある種のテストや計算により価値を証明する必要がある。

レビュアーが簡単に指摘できる警告サイン

ある種のコードは直ちに潜在的な性能問題を誘発する。これは使用している言語やライブラリに依存するだろう。

  • リフレクション:Javaにおけるリフレクションはリフレクション以外の方法で処理するのに比べ低速である。もしリフレクションを含むコードをレビューする場合には、これが絶対に必要かどうかを質問しよう。
  • タイムアウト:もしコードをレビューしている時、操作に対する正しいタイムアウトはどれくらいであるかを知らないだろうが、”タイムアウトに近づいている間にシステムの残りの部分に対してどのような影響があるだろう?”と考えるべきである。レビュアーであるあなたは最悪の場合5分間のタイムアウトに近づいている間にアプリケーションはブロックされていることに気づかなければならない。それが一秒に設定された時に最悪何が起こるだろうか?もしコードの作者がタイムアウトの長さの正しい根拠を説明できず、レビュアーであるあなたが選択された値の長所と短所を知らなければ、それはその影響を理解する誰かを関係者に入れる時である。
  • 並列性:コードは単一の操作を行うために複数のスレッドを使用しているだろうか?性能の改善効果以上の時間と複雑性を持ち込んでいないだろうか?モダンなJavaでは、明示的に新しいスレッドを生成するよりももっと目立たないかも知れない。コードは、並列性の利点を得られない状態でJava 8の輝かしい並列ストリームを使用していないだろうか?例えば、少数の要素に対して並列ストリームを用いたり、とても単純な操作をするストリームだったりする場合、逐次ストリームの操作を行うよりも速度が低下するかもしれない。

適切さ

これらのことはシステムの性能に必ずしも影響を与えるものではないが、マルチスレッド環境における処理の実行に大きく関連するため、この話題に関連している。

  • コードはマルチスレッド環境のための適切なデータ構造を使用しているか?
  • コードは競合状態に影響を受けやすくなっているだろうか?マルチスレッド環境で使用された時にちょっとした競合状態の原因となるコードを書くのは驚くほど簡単だ。レビュアーとして、アトミックでない取得・設定のペアを発見しよう。
  • コードはロックを適切に使用しているだろうか?競合状態に関連し、レビュアーであるあなたはレビュー対象のコードがクラッシュの可能性のある方法で複数のスレッドから値を変更することを許していないことを確認すべきである。コードは同期化、ロック、もしくはアトミック変数を用いてコードのブロックの変更を制御する必要がある。
  • コードに対する性能テストは価値があるか?貧弱なマイクロベンチマークを作成するのは簡単だ。例えば、テストが本番のデータに似ても似つかないデータを使用していると、誤解を招く結果をもたらすことになるかもしれない。
  • キャッシュ:キャッシュは外部へ多数のリクエストを行うことを防ぐための手段となるかもしれないが、導入することで課題も生じる。キャッシュを使用するコードがレビュー対象であれば、いくつかのよく知られた問題を探す必要がある。例えば、キャッシュ項目の不正な無効化などである。

コードレベルの最適化

低遅延のアプリケーションを構築しない大部分の組織では、これらの最適化はおそらく早まった決断の部類となる。そのため、このレベルの性能が必要かどうかを最初に理解すること。

  • コードは必要のない場面で同期やロックを使用しているか?もしコードが常に単一のスレッドで実行されるのであれば、ロックは不要なオーバヘッドである。
  • アトミック変数が使用可能な場面でロックや同期を使用しているか?
  • 不要な時にスレッドセーフなデータ構造を使用しているか?例えば、VectorはArrayListに置き換えられないだろうか?
  • 共通の操作のために性能が貧弱なデータ構造を使用するコードになっていないか?例えば、リンクリストを使用しているが、リスト中からしばしば単一の項目を検索する必要がある場合など。
  • 遅延ロードを行うことで利益があるだろうか?
  • 最初に最も速い評価を置くことでif文やロジックをショートカットできないだろうか?
  • 文字列のフォーマッティングを数多く行なっていないだろうか?より効率的にできるだろうか?
  • ログ記録の際に文字列のフォーマッティングを使用しているか?ログレベルを確認するか、遅延評価するサプライヤーを使用するかのいずれかで保護しているか?

単純なコードが性能の出るコードである

Javaのコードをレビューする際、人間が最適化を行わなくてもよいようにJVMに最適化の機会を与えるためのいくつかの簡単なことがある。

  • メソッドとクラスを小さくする
  • 単純なロジックか?深くネストされたif文やループを使用しない

人間にとってより読みやすいコードであればあるほど、JITコンパイラは最適化を行うために十分な理解を得ることができる。これはコードレビューの間に指摘するのは簡単だ。もしコードが読みやすくクリーンであれば、良好な性能で処理される機会を得ていることにもなる。

セキュリティ

セキュアで堅牢なシステムを構築を行うための労力はプロジェクトの他のことと同じように、プロジェクト自身の性質やどこでそれが実行されているか、誰がシステムを利用するか、どんなデータにアクセスするかなどに依存する。ここでは、コードをレビューするときに見ることになりそうないくつかの領域に焦点を当てる。

可能な限り自動化する

驚くほど多くのセキュリティ確認が自動化できるため、そこでは人間が介在すべきではない。セキュリティテストとして稼働中のシステム全体に対して必ずしも本格的な侵入テストを行う必要はない。いくつかの問題はコードをレベルで発見できる。

SQLインジェクションもしくはクロスサイトスクリプティングのようなよく知られた問題はCI環境で実行するツールにより発見することができる。また、OWASP Dependency Checkツールによりプロジェクトの依存モジュールの既知の脆弱性確認を自動化することもできる。

時々 ”場面に依存する"

イエス、もしくはノーと回答できる心地のよい確認事項がある一方で、潜在的な問題を指摘した上で人間が対処するかしないかを決定したい場面が時々存在する。これはUpsourceが本当に有効な場面である。Upsourceはコードインスペクションを表示し、レビュアーがコードを変更する必要があるか、それとも現在の状況では容認するかを決定できる。

依存関係を理解する

ある領域のセキュリティ脆弱性は第三者のライブラリを通じてシステムやコードベースに忍び寄る。コードをレビューしているとき、本当に最低でもどんな依存関係(例えば第三者のライブラリ)が導入されているかは確認しておきたい。もし脆弱性の確認を既に自動化しているのであれば、新しく導入したライブラリの既知の問題を確認すべきである。

各々のライブラリのバージョンの数を最小限にするように努力すべきである。もし他の依存が追加の推移的依存関係を持ち込んでいる場合はいつも可能とは限らない。しかし、セキュリティ問題に対して他で開発したコードの露出を最小限に抑える最も単純な方法の一つは以下である。

  • 可能な限り使用するソースを少なくし、それらの信用度を理解する
  • 可能な限り最も高い品質のライブラリを使用する
  • 何をどこで使用しているかを追跡し、もし新しい脆弱性が発見されたら、それらを確認することができるようにする

認証すべき新しいパスとサービスがあるかを確認する

webアプリケーションに従事しているが、もしくは認証を必要とするwebサービスや他のAPIを提供しているのであれば、(認証がシステム要件であると仮定すると)新しいURIもしくはサービスを追加する際に認証なしでアクセスできないように保証すべきである。認証が適用されていることを確認する適切なテストをコードの開発者が書いたということを、単純に確認する必要があるかも知れない。

認証が人間のユーザのためのユーザ名とパスワードを使用したものだけではないことも検討すべきである。アイデンティティは他のシステムやアプリケーションやサービスにアクセスする自動処理にも定義する必要がある。これはシステムにおける"ユーザ"の概念に影響を与える。

データは暗号化する必要があるか?

何かをディスクに保管したりやネットワーク越しのどこかに送信したりするとき、そのデータを暗号化するべきかどうかを知る必要がある。パスワードは明らかに平文であってはならないが、データを暗号化するためには多少時間がかかる。もしレビュー対象のコードがデータをネットワークに送信していたり、どこかに保存している、もしくはシステム上に何らかの方法で残しており、暗号化すべきかどうかがわからないのであれば、この疑問に回答できる組織内の誰かを特定しよう。

秘密は適切に管理されているか?

秘密とはパスワード(ユーザパスワードや、データベースもしくは他のシステムのパスワード)、暗号鍵、トークンなどのようなものである。これらはコード内、ソース管理システムにチェックインする設定ファイルに決して記憶されてはならない。これらの秘密はを管理するための他の方法がある。コードをレビューする時は、これらの内容がうっかりVCSに入り込んでしまわないようにすること。

コードはログや監査をしているか?適切に行われているだろうか?

ログと監査要件はプロジェクトによって幅があり、あるシステムでは他のものよりログレベルとイベントのための厳密なルールへの遵守を必要としている。もしログにより何をいつどうやって残すかについての指針があるのであれば、対象のコードがこれらの要件を満たしていることを確認すべきである。もし明確に決まったルールがないのであれば、以下を検討しよう。

  • コードが何らかのデータの変更(例えば、追加/変更/削除)をしただろうか?その変更がされたことを記録するべきだろうか?それはいつ誰が行うべきだろうか?
  • このコードは何かの性能上のクリティカルパス上にあるだろうか?ある種の性能監視システムjに開始点と終点の記録を行うべきだろうか?
  • 記録されるログメッセージのログレベルは適切だろうか?経験的には、"ERROR"というのはどこかが正常に動かない警告が原因となりそうに思える。もしこのメッセージによって誰かを午前3時に起こしたくなければ、"INFO"か"DEBUG"にレベルを落とすことを検討しよう。ループ中や連続して複数回出力されそうな他の場所におけるメッセージは、おそらく製品のログファイルにスパムを送りつける必要はないだろうから"DEBUG"レベルをにするのがよいだろう。

専門家を巻き込むことを忘れずに

セキュリティは重大な話題であり、これは企業がセキュリティの技術的専門家を雇うのに十分な重大さである。もし彼らがいれば、専門家の支援を仰ぐことができる。例えば、彼らをコードレビューに招待したり、レビューしている間に私たちとペアになるために招いたりすることである。そうでなくそのような選択肢がない場合は、システムが有するセキュリティ要件を理解するためにシステムの環境について十分に調査する必要がある(例えば、企業内部のアプリケーションは顧客に対するwebアプリケーションとは異なる側面を持つ)。そうすれば、コードレビューの中で何を探すべきかについてより良く理解することができる。

結論

コードレビューはコードの品質と一貫性を確保するだけでなく、チーム内もしくはチーム間で知識を共有する素晴らしい手段である。基本的な確認を自動化していたとしても、コードと設計について検討すべき側面が多く存在する。Upsourceのようなコードレビューツールは、疑わしいコードに焦点を当てたインスペクションと、特定のコミットでどこの問題が修正されたか、もしくは新しく発生したかを提示する解析を通じていくつかの潜在的な問題の特定を支援する。ツールにより設計と議論をコード内で議論する場を提供し、合意が形成されるまでレビュアーと作者、他の興味のある関係者の間の議論を促進することによりレビューのプロセスを簡単にすることもできる。

最後に、コードの品質について、どの因子が大事かを決定するのはチームである。各コードレビューに対してどのルールを適用するかを決めるのは専ら人間である。また、レビューに参加する全員が、フィードバックを受けたり、コードがレビューを通過するのに"十分"であると最後に合意するための歩み寄りの話し合いをしたりするような対人スキルを身につけて活用する必要がある。

著者について

Trisha Gee氏は様々な業界のJavaアプリケーションを開発した。これには金融、製造、そして非営利のものなど様々な規模の企業のソフトウェアを含む。彼女はJavaによる高性能システムに精通し、開発者の生産性を向上させることに情熱を注ぎ、オープンソースにも取り組んでいる。Trisha氏はSevilla Java User GroupとJava Championのリーダである。彼女はコミュニティを信じており、アイデアを共有することで失敗から学び開発者を成功させることができると信じている。彼女はJetBrainsで開発支援をしている。これはつまり絶えず研究している全ての興味を引く物事を共有するようにしているということである。

この記事に星をつける

おすすめ度
スタイル

こんにちは

コメントするには InfoQアカウントの登録 または が必要です。InfoQ に登録するとさまざまなことができます。

アカウント登録をしてInfoQをお楽しみください。

あなたの意見をお聞かせください。

HTML: a,b,br,blockquote,i,li,pre,u,ul,p

このスレッドのメッセージについてEmailでリプライする

日本語訳のtypo by Yoichi Nakayama

コードレビューは強調的な議論であるべきで→コードレビューは協調的な議論であるべきで

HTML: a,b,br,blockquote,i,li,pre,u,ul,p

このスレッドのメッセージについてEmailでリプライする

HTML: a,b,br,blockquote,i,li,pre,u,ul,p

このスレッドのメッセージについてEmailでリプライする

1 ディスカッション

InfoQにログインし新機能を利用する


パスワードを忘れた方はこちらへ

Follow

お気に入りのトピックや著者をフォローする

業界やサイト内で一番重要な見出しを閲覧する

Like

より多いシグナル、より少ないノイズ

お気に入りのトピックと著者を選択して自分のフィードを作る

Notifications

最新情報をすぐ手に入れるようにしよう

通知設定をして、お気に入りコンテンツを見逃さないようにしよう!

BT