こんにちは。 LayerX バクラク事業部バクラクビジネスカード開発チームEMの高江（@shnjtk）です。

今回の記事では、先日開催された開発生産性Conference 2024にて私の登壇の中でご紹介したコードレビューガイドラインについて、登壇の中ではご紹介しきれなかった部分も含めてより詳しくご紹介いたします。

開発生産性Conference 2024

2024年6月28日〜29日に開催された、ファインディ株式会社様主催の開発生産性Conference 2024、私は2日目のみの参加となりましたが、とても有意義で勉強になるイベントでした。

Tesla共同創業者元CTO、「LeanとDevOpsの科学」の著者来日！開発生産性Conference 2024

基調講演の Dr. Nicole Forsgren さんのご講演とその後のQ&Aもとてもためになりましたし、その後に拝聴した各登壇者様のご講演もバラエティに富んでおり、開発生産性について自分なりに考えを深める良い機会となりました。

私は2日目の最後から2番目のタイミングで登壇させていただきました。資料はこちらにアップしておりますので、もしよろしければご覧ください。ご参加、ご視聴くださった皆様どうもありがとうございました。登壇の機会をくださったファインディ株式会社様にも改めて御礼申し上げます。

speakerdeck.com

コードレビューガイドライン

講演の中で、私が所属するバクラクビジネスカード開発チームで策定したコードレビューガイドラインについて、特にお伝えしたかった点をピックアップしてご紹介しました。

講演後のask the speakerにて、講演の都合上、ご紹介したのは全体のうちの一部であることをお伝えすると、「全体は公開されないんですか？」というお声を何名かの方から頂戴し、ご興味を持ってくださっている方がいらっしゃること、公開しても問題のない内容であることから、本記事にて詳しくご紹介させていただこうと思いました。

策定の経緯

講演資料の中でも触れておりますが、ある日チームメンバーから
「レビュー依頼がアサインされたときに、いつまでにレビューすればいいか分からない」
「どういう観点で何をレビューすればいいか分からない」
という声が上がったことが策定のきっかけとなりました。

このようなレビューに対する不明瞭な点がレビューの遅れにつながり、結果として開発生産性に影響を及ぼしていることが課題であると感じ、チームメンバーで議論しながらガイドラインを策定しました。

ここから、具体的な内容についてご紹介いたします。

ガイドラインの内容

ガイドライン策定の目的

チームでコードレビューを実施するにあたり、レビューの目的やレビュアー・レビュイーに求められる心構え、どのような観点でレビューを行うかについてチーム内で認識を揃える。

ガイドラインの位置付け

あくまでガイドラインであり、強制や義務ではない。重要なのはコードレビューの目的が達成されることである。
チームメンバーの変化や経験の蓄積に応じてガイドライン自体も適宜アップデートされるべきである。

コードレビューの目的

コードベースの品質を高い状態で維持する。
- ここでいう「品質」には、正しく動作すること、テストされていること、パフォーマンスに悪影響がないといったこと以外に、一貫性や可読性、変更容易性といった保守性に関わる側面も含まれている。
プロダクトに実装された機能について、コードレベルで理解している人が最低2人以上いる状態にする。

コードレビューの観点

品質
- 重要な箇所は正しくテストされているか
- テストしやすいコードになっているか
- エラー処理は適切か
複雑さ
- 必要以上に複雑になっていないか
  - 「複雑」とは「コードを読んですぐに理解できない」という意味である
- いきすぎた抽象化がされていないか
- 早すぎる最適化がされていないか
- 将来必要になる「かもしれない」ことのために今は不要な設計や実装がされていないか (YAGNI)
一貫性
- 既存の設計や実装と一貫しているか
- その言語の一般的な慣習やイディオムに沿っているか
  - Goのガイドライン例
可読性
- 変数や関数の命名は適切か。読んで意味が分かるか
- コメントは適切で分かりやすいか。「何を」ではなく「なぜ」を説明しているか
- エラーメッセージは分かりやすいか
知識の共有
- レビュアーは自身が知っている情報をFYIとして記載することでレビュイーだけでなくチームメンバーに知識を共有する
  - 似たようなことを既に他の箇所で実現している場合や、より良い設計や実装方法を知っている場合など

レビュアーに期待すること

Do’s
- レビューは1営業日以内を目処に行う。遅くなりそうな場合は、何かしらAckを返す
- 最も重要な箇所からレビューし、重大な問題や懸念が見つかった場合はすぐにレビュイーに伝える
- 複数の方法が同等に有効である場合、変更を依頼するに足る技術的な事実やデータがない限り、レビュイーの選好を受け入れる
- 自身よりも適切なレビュアーがいると感じた場合は、その人にレビューを依頼する
- 変更の中に素晴らしいものがあれば、賞賛や励まし、感謝の言葉を伝える
- コードの意図が分からない場合は推測せずに確認する
- 変更して欲しいところと変更不要なところを明確にする
  - single commentではなく「レビューを開始する」でcomment/approve/request changeを明確にする
- base branchに取り込みたくない変更がある場合は、request changeをする
Don’ts
- 個人的な好みを押し付ける
- 人格に関わるようなコメントをする
- レビューを複数回に分ける
  - （補足）レビューを複数回に分けることを良しとすると、「途中まで見ました。後でまた見ます」といったような、レビューの引き伸ばしを許容してしまい、結果としてレビュー完了の遅れにつながるので、レビューするときは中断せずに完了させましょう、という意図
- 完璧さを追求する

レビュイーに期待すること

Do’s
- PRは小さく、一つの変更目的だけにフォーカスする
  - 同一タスクを複数PRに分けるときは、少なくとも１名は同じ人がレビューする
- どのような変更(What)をなぜ行ったのか(Why)を書く
  - Whatに関しては、面倒であればPR-Agentを使うとよいです
    - 参考：PR-Agent を使って Pull Request をAIレビューしてみた。（日本語対応もしてみた）
    - ただし、PR-Agentの内容に過不足がある場合は修正してください
- （フロントエンドの場合）変更前と変更後のスクショを貼る
- スプリントタスクにPRのリンクを貼る
  - 目的
    - 後からタスクを見た際に、実装(PR)を確認出来る様にしたい
      - 他の人のタスクの進捗状況を把握したい
      - 関連タスクの実装時や振り返りの際の参考としたい
  - Notion <-> GitHub連携の使用を推奨
    - （補足）バクラクでは、スプリントタスクの管理にNotionを使用しています
- 必要に応じてコード以外にREADMEやNotionなどのドキュメントも更新する
- 1つのコメントに対して1つの修正をcommitする
- レビューして欲しい観点がもしあれば記載する
- 口頭で解決したレビューがあれば、PRに軽くでもよいので記載する
Don’ts
- レビューツール上だけでコードの意味を説明し、コードに反映しない
- コメントを個人の人格に対するものとして受け止める
- レビュアーのコメントのResolve Conversationをする
  - 解決したかを判断するのはレビュアーの責務

レビュアーに期待しないこと

バグの発見
- レビュアーはベストエフォートでバグがないかもレビューすべきだが、レビュイーはレビュアーに対してバグの発見を期待せず、きちんとテストを書くことでバグの混入を防ぐこと
後方互換性が維持されているか (破壊的変更を意図していない限り)
- これもレビュアーによる確認はベストエフォートとし、レビュイーは後方互換性が維持されていることの保証をレビュアーに求めず、きちんと自身で確認すること
大きな機能追加や機能変更の場合に、仕様に沿って実装されているか
- 仕様に沿っているかを確認するためにはレビュアーはレビュイーと同じレベルで仕様を理解している必要があるため、レビューを行うための準備が必要となりレビューが遅くなる
- 仕様に沿っているかを確認する必要があるほど重要なものであれば、仕様検討段階から2名以上で参加するか、あるいはレビュー前に仕様を共有する機会を設けるべきである

セルフマージを許容するケース

基本的にはレビューを通すようにする
セルフマージを許容するケースは以下
- UI表示やエラーメッセージなどの簡単なtypo修正
- CI周りやMakefile、README、テスト追加など、プロダクトの挙動には影響を与えないもの

策定の効果

ガイドラインを策定したことにより、レビューを受けてから完了するまでの目安が設けられたことと、レビューに対する心理的負担が軽減され、レビュー完了までのスピードが速くなったと思います。ここはFactを取っておらず主観的な意見になりますが、Draftや意図的にレビューを保留にしているものを除き、数日間に渡ってレビューが完了しないPRを見かけなくなったので、Factを取ったとしてもそれを裏付ける結果になると思います。ちなみに、この記事を書いている最中にopenなPRを調べてみましたが、一番古いPRで6時間前でした。

おわりに

今回は私の所属するチームのコードレビューガイドラインについてご紹介いたしましたが、今回のガイドライン策定において特に私が大事だと思っているのは、私（EM）のような立場の者がトップダウンで指示するのではなく、チームメンバーからの課題感や発言がきっかけになってこのような開発プロセスの改善が生まれたということです。私にとっても、スクラムの開発チームが目指す自己組織型のチームとはこういうことかというのを実感した良い経験になりました。

バクラク事業部では、このように自分たちで考えプロセスを日々改善しながら、プロダクトを通してお客様へ価値を提供したいと思っているエンジニアを大募集しています。今すぐの転職は考えていなくても、こういう活動に興味があるとか、このガイドラインについてもっと聞いてみたいといった方がいらっしゃいましたら、ぜひお話ししましょう。以下のリンクより、私とのカジュアル面談をお申し込みいただけます。

jobs.layerx.co.jp

また、LayerXではCasual Nightというお料理やお酒を楽しみながらゆる〜くお話ししましょうという催しを定期的に開催しています。原則、招待制となっているのですが、もし興味あるという方は、まずは私までご連絡ください！お待ちしています！

jobs.layerx.co.jp