フツーって言うなぁ!

フツーなサラリーマンのフツーな嘆き.

チームでコードレビューをしているときに考えていること

この記事はなに?

最近、自分の経験や考えていることを言語化してみる実験をしています。 自分で言語化したことのある概念については自信を持って他の人に話せるし、一般化して横展開していくことも可能になるので、メリットが大きいのかなと。

第一弾として、私がテックリードとしてチーム内でコードレビューをしている際に考えていることをまとめてみました。

注意点

  • 思いついたら随時更新します
  • 今回の話のターゲットは主にWebフロントエンド/バックエンドのアプリですが、その他の開発にも使えるノウハウはあると思います
  • ここで書いているのは、仕事でチームを組んだ際にピアレビューをする際の話です。OSS等のコードレビューとは違いがあるかもしれないです

TL; DR

  • 十分な事前準備をすることでレビューの手間を減らそう
  • 目的にフォーカスすることで本質的で建設的なレビューをしよう
  • コードレビューを通してレビュワー、レビュイーともにもっと議論しよう

プルリクエストを出す前にやっておきたいこと(コードレビューで見たくないこと)

以下については、PRを出す前にやっておくべき。

コードレビューの目的と期待値についての認識合わせ

これが一番大事。

これはすべての「作業指示」に共通することだが、いちばん大事なのは指示者(この場合はレビュワー)と作業者(この場合はレビュイー)で事前に作業の目的とその期待値を揃えておくこと。 これが十分でないとあとで揉める原因になるが、意外とできていない場合が多い。

少なくとも、

  • レビューの対象になる機能/バグ対応
  • 何が完了した時点で、どの点をレビューするか/レビュー依頼するか?
  • どれぐらいの変更が入り、それにどれぐらい工数がかかりそうか

あたりについては事前に握っておく。 時間があればissueチケットにまとめておけばいいし、なければチャット等に残しておくとよい(口約束だけだと忘れられやすい)。

ここで、PRの規模が大きくなりすぎないように注意する。

一般に、レビューするコードの行数/範囲が大きくなるとレビュワーもレビュイーもしんどい(イメージとしてはコードの行数に対して指数関数的につらくなる)。 基本的にPRの単位はひとつのフィーチャーを対象とし、それでもフィーチャーが大きすぎる場合は、小さなフィーチャーに分割できないかを考える。 一概には言えないが、変更されるコードが本番コード/テストコード含め800行、ファイル数で30を超えると分割の目安になる(リファクタリングとかでない限りこんなに大きくなることはほぼない)。

例:
X機能の実装(UIデザインのみ)のレビュー。ビジネスロジックと単体テストは範囲に含まず、正常系のUIデザインのみをレビューのターゲットとする。
変更するクラスはXComponentとYService。
1日あたり3.5時間(約50%)の稼働を想定して、初回レビューまで2人日程度を見込む。

また、可能なら、コードレビューで見るべき/見るべきでない点についてのガイドラインを作って先に共有しておくとよい。 このブログ記事も半分ぐらいはそれを意図している。

設計/修正方針に関する打ち合わせ

上とは別に、時間があればおおまかに設計/修正の方針について認識を合わせておくとよい。

  • UIデザイン
  • APIインタフェースデザイン
  • クラス設計
  • ディレクトリ構成

あたりを握っておく。

CIを通す

コードレビューを持続可能な仕組みにするために、レビュワーの負担はできるだけ減らしたい。 人間が見る必要のないもの(見るべきではないもの)については、自動化された仕組みで拾えるようにしておく。

  • コードフォーマットが正しいか?

    Linter/フォーマッタにかける。フォーマットに違反している場合、PRを出した時かビルド時にCIで落とす

  • コードの複雑度と臭い

    静的コード解析にかける。ある程度の基準を設けて開発者間で合意し、それを下回るものはCIで落とす。もちろん、静的解析だけでは全部拾いきれないので、漏れた部分はレビューの対象に回す

  • 単体テスト

    これも、どのクラスを対象としてどの程度のカバレッジ単体テストを行うかの基準を設けて合意し、それを下回るものはCIで落とす。カバレッジ基準については、むやみに高くしすぎるとコードの質に寄与しないテストが増えてしまうことがあるため、うまい落とし所を探るようにする

ルフレビュー

コードレビューをレビュワーに依頼する(= PRを出す)前に、レビュイー自身で自分が書いたコードをレビューするようにする。 これは意外と重要で、実際にやってみると、これだけでもけっこう見落としや間違いが見つかる。

観点は以下のような感じ。

  • PRの形式が正しいか?

    Issueチケットと紐付けられているか、PRを出すターゲットブランチが正しいか、ブランチ名やコミットがあらかじめ合意した形式に従っているか、diffは想定通りか、コンフリクトが起こっていないかなど、形式的にチェックできることについてはチェックし、問題がある場合は修正しておく。これらもCIに載せられそうならCIで落とすようにする。

  • コードが期待通り動くかどうか?

    機能要件を満たさない、つまり「設計書通りに動かない」コードをレビューに出さない/出させない。当たり前のことのように思えるが、特に経験の少ないエンジニアがレビュイーになっている場合は、ケアレスミスや単純な見落としが多いので、基本的な挙動については必ずPR提出前に自分で確認させるようにする。 前述した「レビューの目的と期待値」にもよるが、多くの場合、PRを出す時点では自動化された単体テストまで終了していると思う。ここでの「確認」は正常系のさっくりした挙動確認にとどめ、細かい部分はコードレビュー、スプリントレビュー、結合テスト、QA等で多角的に拾うとよい。

  • PRにおける議論の論点をまとめる

    事前に、「コードを書いていて疑問に思ったこと」、「意見がほしいこと」、「やってもよさそうな気がするけどやらなかったこと」、「やりたかったけどやれなかったこと」について、理由も含めてまとめておくと、PRにおいて議論しやすい。レビュワーに直接伝えてもいいし、PRのコメントとして記載してもよい。 レビューは双方向の議論の場であることを意識し、積極的に意見を出していくと、理解が深まってレビューの価値が高まると思う。

その他、もちろん下記のレビュー観点についても、レビュイーの責任でセルフレビューし、必要があれば修正するかPRコメントに記載しておく。

コードレビューで見たいこと

自分がレビューをする時に気にしている観点をざっとまとめておく。 まだ足りない点やまとめきれていない点があるので、見つけたら随時追加する。

PRの形式

  • Issueチケットとの紐付け
    • あとでコミット履歴を見たときに変更理由が明確なようにしておく
  • ブランチ戦略との整合性
  • Descriptionの妥当性(事前に合意したことをやろうとしているか?)
  • コミットが適切か?
    • コミットメッセージ、粒度は適切か?
  • コミットのコンフリクトがないか?

PR内の事前質問についての回答

レビュイーから質問や意見を求めるコメントがあれば、それに回答する。再レビューの場合は、以前に行った指摘が意図通りに解決されているかについても確認する。

機能要件を満たしているか

前述の通り、ここはレビュイーによるセルフレビューにより解決できていることを期待したいが、設計についての認識違いや実装の瑕疵など、拾えそうなものについては拾う。 一行一行見ていくと時間がかかるため、実際に開発環境等で動作させてチェックするとよい。

非機能要件を満たしているか

上述の通り、コードレビューでは、主に非機能要件についてのレビューを中心にしたい。

以下、思いつく限りの観点を箇条書きで書いていく。

非機能要件については、IPAの非機能要求グレードを参考にしている。

www.ipa.go.jp

可用性について

  • エッジケースを適切にハンドリングできているか?
    • null値/空文字/空集合/デフォルト値
    • 境界値(0、負数、intの最大値とそれより大きい数など)
    • 型変換(intの変数に文字列を入れたときどうなるか?)
    • ネットワークエラー/IOエラー/DBエラー
    • 非同期処理によるエラー(排他制御のミス、デッドロックなど)

パフォーマンス(性能・拡張性)について

  • (動作はするが)現実的でない実行時間(≒ループ回数)になっていないか?
    • 例えば、通常のプログラミング言語 10^{9}回以上ループが走ると、実行に10秒以上かかる可能性が高く、Web APIであればほとんどの場合非機能要件を満たせない
  • 適切なアルゴリズムとデータ構造が使われているか?
    • 例えば、HashMapを使えばO(1)で検索できるところをArrayListで検索していないか?
    • もちろん拡張性とのトレードオフはあるので、場合によっては遅い方を選ぶこともある
  • 現実的でない量のメモリを確保する実装になっていないか?
  • 現実的でない量のスレッドを作成する実装になっていないか?
  • スレッドセーフか?
  • 利用したメモリをアプリのライフサイクルの中ですべて回収しているか?メモリリークしていないか?
  • アプリが吐き出すログやその他のファイルの分量は適切か?ログ溢れが起こらないか?
  • DBへのコミットの単位は適切か?
    • 特にバッチ/ワーカーアプリにおける逐次コミットは遅さの原因になることが多い。バルクアップデートの手段がないか検討すること

可読性(運用・保守性)について

  • クラス/メソッド設計
    • 単一責任の原則(SRP)を満たしているか?神クラスになっていないか?
    • DRYを満たしているか?まったく同じことを実現しようとしているのにクラス、メソッド等を分けていないか?
    • クラス/メソッド/変数のスコープは妥当か?
      • 例えば、ローカル変数で十分なことをインスタンス変数で行っていないか?
    • 1メソッドで実行しようとしていることが大きくなりすぎていないか?
      • 特に、メソッドの行数が多すぎたり(1メソッド50行以上とか)、メソッド内の条件分岐が多くなりすぎたり(ネストが3階層以上とか)していないか?
  • コーディング規約
    • クラス/メソッド/変数の命名は妥当か?
      • 命名はコーディング規約に沿ったものか?
      • 名前が体を表しているか?
    • コードコメントは適切な内容か?
      • コードを読むだけでは意図のわかりづらい箇所にコードコメントを入れてあるか?
        • 特に、暫定対応をした箇所には対応するチケット番号を書いておくとよい
      • コードコメントの量は必要十分か?また、内容が陳腐化していないか?
        • "How"(どのように実現するか?)ではなく"Why"(なぜこの実装になっているか?)や"Why not"(なぜ他の実装になっていないか?)をソースコードにコメントするようにする。Howはソースコードを読めばすぐにわかり、かつ頻繁に陳腐化するのに対し、他はコードに残らない重要な情報になることが多いため。
        • 不要なコードコメントは減らし、できるだけクラス/メソッド/変数名に伝えたい情報を詰め込むようにする
        • コードに残す必要のない、議論の過程などについてはコードコメントではなくPRコメントに残すようにする
  • ログ
    • ログレベルは適切か?
    • ログ文言は事象を表すわかりやすいものになっているか?エラーログの場合、エラーが発生した場所と時間、原因がログから追跡できるようになっているか?
  • テスタビリティ
    • メソッドが不要な副作用を持っていないか?
    • イミュータブルな変数/メソッドを利用しているか?
    • ユーザからの入力値や現在時刻、ランダム値など、自動テストの実施において障害になる値を外部化できているか?
  • 自動テスト
    • 単体テストが十分にロジックを網羅しているか?
    • タイミングによってテストが通ったり落ちたりするようなつくりになっていないか?
      • 特に、非同期処理や時刻(現在時刻、月末、うるう年、タイムゾーンなど)の扱いが正確にできているか?
    • 単体テストの量は必要十分か?
      • テストケースとして重複しているテストや、コードの質に寄与しないテストがないか?
  • 設定、環境構築
    • 容易にアプリのビルド/動作確認ができるようになっているか?複雑な手順が必要になってしまっていないか?
    • 設定値の必要十分な外部化ができているか?
      • ステージング環境がある場合、容易に設定をステージング環境向けに切り替えられるか?

移行性について

  • 捨てやすいコードになっているか?
    • 例えば、Web APIにおけるコントローラ、Webフロントシステムにおけるコンポーネントなどが、今後の運用の中で通常の使用に即した分割になっており、不要になったり、システム分割したりする際に容易になっているか?
  • リリース後に問題が発生した場合、ロールバックが可能な実装になっているか?
  • 後方互換性が必要な場合、それを担保できているか?
    • 旧バージョンを廃棄することが決まった場合、容易に可能か?

セキュリティについて

  • 最新版のプログラミング言語/フレームワーク/ライブラリを使っているか?
    • 改修の場合は、影響範囲を見ながらバージョンアップするか都度判断する
  • ミドルウェアのユーザ名/パスワードにデフォルト値や安易な値を使っていないか?
  • 入力値のバリデーションが十分であるか?
  • (特に通信がインターネットを介す場合、)安全な通信を利用しているか?
  • クレデンシャル(パスワードや秘密鍵等)が外部に露出していないか?
    • クレデンシャルについては、可能なら設計時に別リポジトリで管理し、アプリ実行時に値を注入するようにする
  • 技術詳細など、ユーザにとって不必要な情報を外部に露出していないか?
  • 典型的な攻撃(DoS/XSS/CSRF/SQLインジェクション/コマンドインジェクション/ディレクトリトラバーサルなど)に対処できる形になっているか?

設計指針に合致した変更か?

プロジェクトとして文書化された設計指針があるのであれば、それに従っているかもここで見る。 なければ、テックリードの責任で変更がコードベース内での一貫性を保っているかを確かめる。

必要十分な変更か?

「PRの目的を達成するために必要な変更は余さず入れる」、「PRの目的を達成するために不要な変更は何も入れない」という観点も重要。

例えば、

  • 動作させるのに必要な設定変更がコミットから漏れていないか?
    • 例1:NPMのpackage-lock.json
    • 例2:バージョンを上げるために必要なファイル(AndroidManifest.xmlのversionCode)
  • 余計なファイル/クラス/メソッド/変数/ログ等が含まれていないか?
  • ファイルの実行権限等が必要十分か?

など。

特に、既存のコードを変更/削除するPRの場合は、他に変更漏れ/消し忘れがないか気をつける。レビューツールには変更のあるファイルのdiffしか出てこないので、これはコードレビューの観点として忘れずやった方がよい。

また、PRの目的に合致しないが有用と思われるリファクタについては、面倒でも新しくPRを作って入れてもらうようにする。

プルリクエストを承認する前にやっておきたいこと

テックリードとしてPRを承認し、マージする際には、以下を確認しておくとよいと思う。

  • 指摘対応がすべて終了しているかを確認する
  • レビューの結果見つかった残作業やさらなる改善点がある場合は、レビュイーと話した上で、必要であればチケット化しておく
  • 議論の記録がたどれるようにしておく
    • 議論の結果決まったことがあれば、issueチケットかPRのどちらかに記載しておく

その他気をつけたいこと

その他、レビューをしていく中で培った私見をざっとまとめておく。

目的に沿ったレビューをする

自分自身、意外とやってしまうのは、PRの目的にそぐわない指摘をしてしまうこと。 あまり関係ない指摘を続けてしまうと、それがある種の正論であったとしても、手戻りによる工数が膨らんだり、レビュイーを必要以上に混乱させることになる。 事前に握った目的外の指摘は最小限に留めるようにする。

明らかにスケジュール内で実装不可能でありリリースのブロッカーにならないものについては、それを明記した上で「今後の課題」とする、あるいは別のチケットに切り出すことを考えるべき。 実装のボリュームにもよるが、レビューの手戻りのピンポンが恒常的に3往復を超えていると、プロセスに何らかの問題がある可能性が高い。

レビューは双方向の議論の場

上でも述べたが、レビューは双方向の対等な議論の場であるという意識が必要だと思う。

ありがちなのは、コードレビューは開発プロセスに含まれているが、レビューが承認を取る/渡すための作業になってしまい、レビュワー、レビュイー双方の学びの機会が失われてしまうパターン。 特にレビュワーとレビュイーにプログラマとしての実力差が大きい場合、レビュワーが一方的に指摘し、レビュイーは言われた点をひたすら修正するだけの作業になりやすい。

レビュイーは自分が書いたコードの意図や設計についての疑問、その他気になったことは何でも積極的にレビュワーに話すようにするとよいし、レビュワーの指摘が正しくないと感じたときは反論してよい。 レビュワーは、コードレビューを自分が担当しているコードや周辺技術についての学習の機会、また、コードの質とチーム全体のスキルを上げていくための重要な工程だと認識し、レビューしていて気になったことについては調べ、思ったことを積極的に指摘するようにする。

指摘は論理的、客観的、具体的にする

コードレビューにおいて指摘をする際は、「論理が明快で、誰もが納得でき、何をどうすればよいかが明確になっている説明」をするようにする。 言うのは非常に簡単だが、実行するのは簡単なことではないし、自分自身、完璧なアイデアがあるわけではない。

以下に自分がレビュワーとして試していることを記す。

  • 「この方がキレイ」など、本人の主観によるような表現は避け、論理的な説明になるようにする。必ず指摘の理由を説明する
  • レビュイーの背景知識を考慮し、言葉遣いに反映する
    • 特に、馴染みのない概念を説明する際には、具体的な例と、それをすることで何がいい/悪いのかを明示する
  • 指摘の際、持論を補強するための参考文献を貼る
  • 自分が少しでも理解できていない点があるのであれば、「指摘」ではなく「質問」の形式を取る。相手の意図を確かめた上で、自分の意見に反映する
  • 修正方法が説明しづらいと感じた場合、指摘コメントの中に具体的なコードスニペットを書く
  • 具体的な修正箇所を明示しておく
    • レビューツールで該当箇所へのURLが取得できるなら、コメント内でリンクを張る
    • 同一の指摘が複数箇所に当てはまる場合、「ここも同様です」を記載しておく
  • 修正の必要がないものについては、「修正不要です」と明記しておく

お互いに理解し、理解されるような説明を心がける。

テキストコミュニケーションに頼りすぎない

レビューツールやチャットツールなど、テキストでのコミュニケーションは、非同期的に行うことができ、かつ記録が残るので非常に便利であるが、一方で難しさもあると感じている。

特に、

  • 上で述べた通り、自分の指摘意図を完全に相手に伝えるのは概して難しい。特に非同期コミュニケーションの場合、相手が指摘を理解しているかどうかを確認する術がない。理解を誤っていた場合、その分はすべて手戻りになる可能性がある
  • 意見が対立した際に加熱しやすい

などの問題がある。

結果的に5分話したら解決した、というケースも結構あったので、「うーん、伝わってないなー」と感じたら、面倒がらず相手と直接話した方がよい。 ペアプログラミング、モブプログラミングのように、時間を取って一緒に画面を見ながら話すのも有効だと思う。

参考文献

開発現場に学ぶ、円滑なコードレビューに必要な8つの手法 ~手段から準備、実施時期まで徹底解説~ - エンジニアHub|Webエンジニアのキャリアを考える!

www.jpcert.or.jp