テックリードが実践する「気持ちよくレビューできるチーム」の仕組み

はじめに

私は、現在自社開発企業においてテックリードとして働いています。この記事では、コードRvで発生する問題を中心にとらえながら、チーム開発で楽しく開発する上で重要な共通認識づくり、仕組みづくりについてご紹介いたします。

目次

• コードレビューでよくある問題
• 筆者が目指すチームの状態
• Pull Requestを作る前に。共通理解が成功の8割を決める
  • Why, Whatの共通理解
  • Howの共通理解（どのように作るべきか）
• 実装: 個人によるコードのばらつきを排除する
• レビュー: レビュアーとレビュイーが一緒に問題を解決する
  • レビュイー編
  • レビュアー編
• テスト: レビューと品質保証
• まとめ
  • PR-個人開発アプリ

コードレビューでよくある問題

私が考える理想の開発チームは、メンバーが楽しく開発でき、届けた機能がユーザーの価値に結びついている状態です。またメンバー間の実装品質の差が小さく、Rvの手戻りが少ないことが理想です。

しかし現実には、こうした理想を妨げる問題が発生しがちです。コードRvで発生時には下記のような問題がしばしば発生します。

• 好みの問題
  • コードの書き方でケンカになる
• 批判的なレビュー
  • 悪気は無いのに文章が攻撃的に見える
  • 「あの人のレビューは受けたくない」
• レビュー無限ラリー
  • 小さな指摘が何度も続き、なかなかマージできない
  • 正直、「早くマージさせてくれ…」と思う
• 大きな手戻り
  • 数千行の巨大Pull Requestが突然出てきて、しかも実装の根本が間違っている

こうした問題はコードRvそのものではなく、その前段階で失敗していることが多いです。例えば、

• チーム内で業務知識の理解度の差が大きい
• アーキテクチャや実装方針が明文化されていない
• レビュアーとレビュイーのコミュニケーションの質が低い

つまり、コードRvの問題は枝葉の現象であり、根本原因は開発プロセスにあると考えます。
この記事では、気持ちよくレビューできるチーム文化を作るために、その背景にある問題をどのように解決するかを考えます。

筆者が目指すチームの状態

私が大切にしているのは、次のような状態を実現することです。

• 業務ルールや背景をチーム全体で共有し、同じ理解を持てること
• 誰が書いても同じ品質・同じ構造になるような仕組みを整えること
• レビュアーとレビュイーが対等で、協力して問題を解決できるチームであること

この記事では、上記の状態を実現するために、開発フロー全体をどう設計するか に焦点を当てます。紹介する順序は次のとおりです。

• 開発前の準備
• 実装
• コードレビュー
• テスト

この流れに沿って、レビュー文化を良くするポイントを整理していきます。

Pull Requestを作る前に。共通理解が成功の8割を決める

コードRvの話の前に、そもそもPull Requestを作り始める前の準備が重要という話をします。何事も準備8割です。この章では、Why, What, Howの理解について述べます。

• Why, Whatの共通理解 ... なぜ作るのか？ユーザーのために何を作るのか？
• Howの共通理解 ... どのような設計で作ると、保守性の高いコードになるか

Why, Whatの共通理解

チームは、業務ルールやユースケースを理解できていますか？

コードを書き始める前に、作成する機能に対する業務要件やシステム要件がチーム内で共有されているかが大切です。エンジニアはHowに意識が向きがちですが、なぜ作るのか？(Why)の方が100倍重要です。そこで、コーディングを始める前に要件を文章で残す習慣を作りましょう。

私たちは、機能実装の前に日本語のドキュメントを書いています。理由は、ビジネスサイドとの共通理解を作るためです。プログラミング言語は How は書けますが、Why を説明するのは難しいです。そのため、日本語のドキュメントがプログラミング言語を補い、共通理解を助けてくれます。

シンプルな要件定義書をGit管理する

ドキュメントは下記のようなmarkdown形式とし、業務要件、システム要件、非機能要件を書き下します。実装Sprintの前の段階でドキュメント化しておくと、のちに説明する「タスク分割」もスムーズになります。アジャイルの場合、ウォーターフォールのようなガチガチな要件定義書を作る必要はないですが、ビジネスサイドとの合意形成の証拠を取るという意味でも文章で残すことは重要です。

※ 画像は架空のものです

用語集を作る

要件定義書とは別に用語集を作ることもお勧めします。下記のように日本語、英語、説明の3列がある形にします。ビジネスサイドとエンジニアで、各用語のブレを小さくし、共通認識を持つためです。日本語、英語の対訳があることでソースコード内の単語のブレを小さくすることも期待できます

Howの共通理解（どのように作るべきか）

設計やアーキテクチャに関しての共通理解

システム設計において、どこに何を書くべきか？何の責務を持たせるか？といった共通理解も重要です。この理解がないと巨大クラスや責務があいまいな処理が増え、コードの可読性が低下します。

• 例
  • 単体テストの書き方
  • ログのルール
  • 例外、エラー設計
  • リポジトリ層、ユースケース層の責務

そこで、私のチームではコーディング規約をREADMEに記載するとともに、Clean Architectureのレイヤールールに従った開発を行っています。これらのルールセットはドキュメントで明文化しており、Confluence上で誰でも閲覧できる状態にしています。また、PRのレビュー議論やレトロスペクティブで出てきた内容をドキュメントにも反映し、メンテナンスすることで、現場の実態に応じたルールとして維持します。

※ コーディング規約を作り、チーム内で回覧する（図はサンプル）

チームのタスク分割スキルを底上げする

チーム内でPull Requestが大きすぎて、コードが読みにくい原因の一つに、タスク分割のまずさがあります。人によって1回のPRで実装する機能のスコープが異なるためです。スクラム開発においてはSprint Planningで、ウォーターフォール開発ではWBSでの作業粒度を小さく揃えましょう。

タスク分割は人によって差が出る箇所なので、最初はできるだけ会話をしながら分割します。例えば、あるジュニアメンバーに一つの機能をアサインし、実装前に1日以下の作業工数になるようにタスク分割のたたきを作成してもらいます。その後MTGでたたきを確認し、質問をしながらタスクの粒度を揃えます。これを何度か繰り返していくと、メンバーもコツをつかみ、タスク分割がうまくなります。

---

リーダーの心得
このフェーズでは、チームの認識合わせ、コミュニケーションの促進など、エンジニアとしては気が進まない内容が多いです。
リーダーは大変ですが、チームメンバーの会話が増えるように旗を振りましょう。ここで頑張ると、後の開発フェーズが安定し、十分すぎるほどのお釣りが来ます。

---

実装: 個人によるコードのばらつきを排除する

前章で開発のための共通理解が重要という話をしました。この章では、実際にコードを作成するうえでのTipsを記載します。

コードフォーマッター、リンターを強制する

CI/CD

Github Actionsでの自動実行を必ず設定し、通らなければマージできないルールを作ります。これによってコードのスタイルに関する不毛な議論が少なくなります。また、人から指摘されるのではなくシステムから指摘されるため、イライラの矛先が人に向かなくなり安全です。

ローカル環境

開発環境で保存時に即座にフォーマットを書ける仕組みも必須です。VSCodeでは Format on Save という機能があり、ファイルをCtrl + Sで保存した瞬間に自動でコードが整形されます。Format on Save を導入する前は、最後にコードを整形するコミットができ、無駄なコード差分が発生していました。導入後、コミットの変化点が小さくなり、読みやすくなったと感じます。

※ これは個人開発コードです

Githubの Pull Requests テンプレートを設定する

コードRvを依頼するときに、Pull Requestに何も説明を書かないエンジニアをそれなりに見てきました。Pull Requestの概要欄には、変更の目的、レビュアーに見てほしい点、心配な点などを書き、レビュー負荷を下げる目的があります。この部分も個人によるばらつきが大きいため、下記のようなテンプレートを設定いたしました。

※ これは架空のプロジェクトです

リファレンス実装を作る

現場でのコーディングを観察していると、「まず既存実装をコピペ」してから機能追加する人が多いです。つまり、参考となる既存実装の品質が悪いと、質の悪いコードが複製され、それが暗黙のコーディング規約になるのです。このような悪い実装が広がると、レビューで指摘しても「他の実装がそうなっているから」と難色を示されることがあります。そこでレビュアーが負けると、更に質の低いコードが増殖します。

これを防ぐために、私はプロジェクトのできるだけ早い段階で、ハイスキルのエンジニアが本気で書いたリファレンス実装を作るべきと考えます。

• APIならPOSTメソッド一本
• 画面なら、大きめのコンポーネント一つ

で十分です。リファレンス実装を作ったら、これをチームに共有し、意見をもらいつつこの実装を参考にするようにします。これにより粗悪なコードが複製される速度を遅らせます。

---

レビュー: レビュアーとレビュイーが一緒に問題を解決する

ここまでの章では、開発プロセスの土台およびコーディングの仕組みづくりについて説明してきました。ここからは、レビューの場面で意識すべきポイントを、レビュイー向け、レビュアー向けに分けて具体的に解説します。

レビュイー編

まず、コードを作成し、レビュー依頼を出す人に向けてのコメントです

to レビュイー: 完璧主義はNG! 2割共有をしよう

中にはコードRvまでに100点満点の完璧なPull Requestを作ってからコードRvを受けたいエンジニアもいます。しかし、PR作成においても完璧主義は仇となる場合が多いです。特に複雑な機能追加において、実装、単体テストまで完璧に作り上げたものの、そもそもの実装内容がチームの実装方針と合わず、ほとんど作り直しになるケースをしばしば見かけます。これはレビュイーが成果物の途中経過を開示せず、一気にドカンと出してしまうせいで、認識齟齬に気づくのが遅れるためです。レビュイーのモチベーションにとってもよくありません。

Draft Pull Request を活用する

私は、複雑な機能を作成してもらうときは、まず2割でDraft Pull Requestを作ってもらうようお願いしています。Draftでは、単体テストは無しで、コードは動かなくてOK、アプリコードの方針だけ説明できる状態です。この状態でレビューもしくはペアプロを挟むことで、初期の段階で認識齟齬が減り、コードの品質も大幅に上昇します。OKであれば完成させてもらってから、再度Rvを行います。

小さなチェックポイントを挟むことで手戻りが減り、100%にしてから差し戻すよりもお互いに気持ちよく開発することができます。

to レビュイー: 1つのPRにおけるコード行数は数百行以下に

基本的に、実装とテストコードをセットにしてPRの変更サイズは数百行にすべきです。理由としては、変更行が大きいとレビュアーの負担が大きく、レビューの深さが浅くなります。本当にレビューすべき箇所や、品質を作りこむ必要のある箇所がスルーされ、徐々にコードベースが汚くなる傾向があります。
よって、一つの大きなPRをを出すのではなく、小さなPRを複数行出した方がコード品質も良くなります。CIにおいて、1000行以上のコードはマージできないようにするという強いルールを設けるのもアリです。

---

レビュアー編

次に、コード確認し、レビューを行う人に向けてのコメントです

to レビュアー: 巨大プルリクを作る人にはタスク分割を指導する

レビュイー編で述べた内容ですが、一度に巨大なPRを作る人は、作業タスクの分割が下手な人が多いです。この場合、「次からはPRを小さくしてください。」と伝えても効果が薄いことが多いです。
なぜなら、巨大プルリクを作る人は、タスク分割をする勘所を把握しておらず、同じことを繰り返す傾向にあるからです。そこで、一度レビュアーもしくはリーダーがその人と伴走しながら、タスク分割を支援するのが効果的です。

体験談

あるエンジニアの方で、機能実装のとき数千行のPRを提出する人がいました。PRタイトルは「〇〇機能の実装」。一つのPRの中にあれもこれも詰められていてレビューが非常に困難な状況でした。次のPRからは、その人と会話しながら

• 「モックレスポンスだけ返すAPIの作成」
• 「テーブルクエリの作成」
• 「更新処理ユースケース層の実装」

のように複数のサブタスクに分割しました。分割後のPRのサイズは必然的に小さくなり、また変更範囲も明確でコードRvがしやすい状態になりました。

toレビュアー: 批判をするなら代案を出すこと

レビューコメントの中には「〇〇はダメです。書き直してください。」という現状の批判しか書かない人がいます。コードレビューを受ける側としては、

• どのような理由でダメなのか？
• どのように書き直すべきか？

が伝わらないため、レビュアーの正解を察するゲームになります。

レビュアーは「察してちゃん」になってはいけません。

私はレビュー時にNGな理由と、どう直すべきかを必ず書いています。これにより指示が具体的になり、レビューが伝わらない！というリスクも減らせます。

❌ 悪いレビュー例

  処理が分かりにくいので直して下さい。

→ 怒ってるのかな？ 💬

  他のコードに同じ実装あったと思うので同じようにしてください。

→ どのように直せばいいか分からない... 💬

🟢 良いレビュー例

[IMO]
こちらのコードですが、if文の条件が複雑で、異なる関数内で同じ条件分岐が別々に定義されています。
if文の条件をUserオブジェクトのメソッドとして実装し、
この関数からはそのメソッドを呼び出すように修正をお願いできますか？
下記のようなイメージです。

# BAD:
if user.age > 20 && user.license is not None && user.accident is None:
    # do something

# ↓

# Good:
# ゴールド免許ドライバーか?
if user.has_gold_license():
    # do something

to 全員: レビューのやり取りが2往復目になったらペアプロしよう

テキストベースでのコードレビューで何往復もラリーが続き、レビューが終わらないというケースはありませんか？

無限ラリーになる原因は、文字では曖昧なニュアンスが伝わっておらず、認識のずれが解消されていないからです。

私のチームでは2往復目になったら、30分くらいペアプロをします。Webミーティングで画面共有しながら、レビュアーが考える修正後のコードイメージを書いていきます。レビュイーはそのコードを見ながら、疑問点や代案があれば代案を伝えます。30分程度の会話で方針が定まったら、PRのレビューに決まったことを箇条書きにし、レビュイーに差し戻します。途中でペアプロを挟むことで、微妙なニュアンスが伝わり、お互いの認識のずれも見つかります。私は常時ペアプロは嫌いですが、レビュー時にペアプロを入れるのはとても良い体験になります。

---

テスト: レビューと品質保証

テスト工程は、レビューと同じくらいコード品質に影響します。ここでは、レビュアーとテスターの役割は違うことを述べ、単体テスト、結合テストの考え方について述べます。

レビュアーはテスターではありません。

レビュアーの皆さん、レビュー時に動作確認まで行って疲弊していませんか？

品質保証の責任はコードを書いた人自身にあります。一方でPR作成者が何をもって動作確認をしたかを伝えるため、Pull Request テンプレートにチェックボックスと証跡欄を設定しています。これによりレビュアーが動作確認をするのではなく、テスト観点のチェックと結果の確認のみに集中します。また、レビュイーの提示した動作確認が甘ければ、動作確認を追加で依頼することもできます。これにより、レビュアーは、手を動かさず判断や指示を行い、レビュイーにテスト実施を委譲します。

単体テストと実装はセットで、同じPRに必ず書く

もはや当たり前のことですが、単体テストが書かれていなければレビューを差し戻しにします。「テストコードは次のPRで書きます。」という人がいますが、絶対に通しません。PRを分けると他のタスクが挟まって、テストを追加するのを忘れるからです。また、PRは常にコードが壊れていないことを確保した上でマージすべきだからです。

※ テストを書くとPRが大きくなる場合は、そもそもタスクが大きすぎることが原因です。タスク分割をし、コード変更行を1000行未満に保ちましょう。

カバレッジ100%神話は捨てる

カバレッジツールは、テストの通っていない箇所を明らかにし、条件網羅をチェックをするのに有効なツールです。
ただし「カバレッジ100%」にする必要はなく、私はC1分岐で90%以上あればOKとしています。理由としては下記が挙げられます。

• 手段の目的化がおこる
  • カバレッジを100%にするために無駄なテストが書かれる
  • ボイラープレートなど本質的でない行をカバーするためのテストが書かれ、テストコードが肥大化

テストコード行が膨れると可読性が下がり、どれが重要なテストで、どれが重要でないテストか分からなくなります。重要なのは、テストの行数ではなく、重要な業務ロジックが正しく検証されているかです。カバレッジツールはあくまでもガードレールであり、カバレッジ100%をKPIにしてはいけません。

システムテストを薄く自動化する

単体テストが書かれていても、実際に一つの機能として動作させると思わぬバグが見つかることがあります。それを防ぐために、うすいシステムテストを書くことをお勧めします。デプロイのたびに実行されるので、手動テストを繰り返すよりもコスパが良いです。

私のチームでは、API一本につき、正常系の1~2ケースのみを記述したリクエスト&レスポンスベースのテストを書いています。一方で、システムテストは壊れやすく、メンテナンスも面倒なため、異常系のテストは書かず、手動での動作確認にします。このうすいシステムテストのおかげでリリース後にAPIが落ちて使えないという不具合が大幅に減りました。

※ なお、システムテストはデバッグが難しく保守コストも高いため、テスト数は少なく保ってください。単体テストと同じように完璧主義を目指すと、保守で詰みます。詳しくは「テスティングピラミッド」という概念を検索してください。

まとめ

今回紹介した内容は、今までの開発チームで実践した改善活動をまとめたものです。

改善活動

• 業務ルール、コーディング規約に関する共通理解を作る
• レビューアーとレビュイーのコミュニケーションの質を上げる
• 自動化できるところは自動化する

改善活動により、下記のような変化が見られました。

チームの変化

• 好みの問題が減る
• 指摘が柔らかくなり、攻撃的に感じられにくくなる
• レビュー往復・手戻りが減る
• チーム全体が「前に進むレビュー」を実感できる

実践のコツとしては、自動化できるところは自動化してしまう。人間の判断が入る箇所は共通認識を作る。という地道な作業の積み重ねです。特にチームの文化を変える作業は時間がかかり、メンバーからの反発を受ける時期もあるでしょう。しかし、気持ちよく開発するチームを作るというゴールを目指していれば、3か月、半年、1年経つ頃には見違えるほどチームの開発状況は向上するはずです。

この記事が、チームの開発体験向上に貢献できれば幸いです。

---

PR: 個人開発アプリ

レシートをスキャンするだけで、レシートのファイル名を自動で生成するAIアプリを作成しました。ログイン不要ですぐに使えますので、1分間だけ使ってみてください！

仕事の進め方のnote記事