はじめに
テクノロジカルマーケティング部データプラットフォームグループでグループリーダーをしている富士谷です。普段は、データ収集基盤 Livesense Analytics とデータ活用基盤 Livesense Brain の開発を行い、マネジメントもしています。
さて、今回は、データプラットフォームグループの「レビューに対する考え方」をご紹介したいと思います。なお、あらかじめ、お断りしておきたいのですが、リブセンスの開発チームすべてが同様の考え方とは限らず、あくまで、あるチームの一例であることをご了承ください。
本題に入る前に少し背景を説明しておこうと思います。
ざっくり2年ほど前は、データプラットフォームグループは、データ収集基盤を担当するチームとデータ活用基盤を担当するチームに分かれていました。この2つのチームでは、利用技術（例えばAWSとGCP）、課題管理ツール（JIRA とGitHub Project）、設計思想、レビューに対する考え方まで、様々なところで違いがありました。今後、チームの垣根を超えて、データ収集や活用の改善をしていきたいと考え、徐々にチームを統合していきました。現在は、各々が必要に応じてお互いのシステムの改変を行える状態に近づきつつあります。すでに、GA4を活用したリプレイスなど、チームをまたいだLAの大規模な改変も実施しています。
レビューに対する考え方も揃ってきており、大きな課題感はありません。が、これまでチームに対しては、チーム間の差異に焦点をあてた説明しかできておらず、レビューに対する考え方について明文化できていませんでした。「レビューの心得」なるものを明文化しておけば、新任者にも有用ですし、別チームへの説明にも便利だと考え、社内向けドキュメントにまとめて公開し、これをベースに本記事を執筆しています。
レビューに対する考え方
ここから本題です。はじめに、社内向けドキュメントの構成と同じくいくつかの参考資料について抜粋しつつ紹介したあと、レビューにおいて心がけてほしいことをまとめています。主には、GitHubのプルリクエストに対するコードレビューを対象として想定していますが、ドキュメントなど継続的に改善しやすい成果物のレビューにも適用できると考えています。
参考資料
Google Engineering Practices Documentation
google-engineering-practices.translation.shuuji3.xyz
コードレビューについて端的にまとまった非常に良い資料です。いくつかページがありますがどれも必読です。正直、これを読んで、現在のチーム状況に合わせて抜粋すれば心得としては十分では、とも思います。 なお、文章中の「CL」とは「Change List（変更、パッチ）」を指します。
コードレビューの基準 | google-engineering-practices
一般に、ある CL が作業対象のシステムのコード全体の健全性を具体的に向上させる状態に一度でも達したならば、たとえその CL が完璧なものでなくても、レビュアは承認を賛成しなければならない。
「Done is better than perfect」とも近いようにも思います。よりよいものであれば、完璧でなくても承認しよう、前に進もう、継続的に改善しよう、という考えです。
対立が解消しない場合、最も一般的な方法は問題の解決を上位レベルに移譲することです。よくある移譲の経路としては、より広いチームディスカッションを行う、TL に仲裁に入ってもらう、コードのメンテナに決定を求める、Eng Manager に助けを求める、といった選択肢があります。CL の作者とレビュアが合意を得られないという理由で、CL を放置してはなりません。
我々のチーム内でも稀に、レビュワー、レビュイー間で意見の相違が起きます。どちらか、または双方の認識不足が原因であることも多いため、口頭で確認したほうが早い場合は、Slack のハドルミーティングの機能を使っています。これでも解消されない場合は、マネージャーが間に入り、方針を決定します。場合によっては、マネージャー判断で、システムごとに定めたコードオーナーに判断を移譲することもあります。
コードレビューのスピード | google-engineering-practices
コードレビューのスピードについての記事です。コードレビューが遅いと、チームの全体の速度が低下する、コードレビューしたくなくなる、コードの品質が悪くなる、などの悪影響があります。集中を阻害してまで早く行う必要はありませんが、1営業日中までの回答が望ましいです。個人的には、スピードの部分は、マネージャーによる意識付けが重要だと思っていて、以前は、口酸っぱく言ってました。
コードレビューのコメントの書き方 | google-engineering-practices
やさしさを持ちましょう。あなたの考えの論理を説明しましょう。問題を指摘して明確な方向性を示すことと、開発者自身に決定させることのバランスを取りましょう。 開発者に対して、コードをシンプルにしたりコードにコメントを追加してもらうようにしましょう。
私がレビューするときには、指示する（「〜〜してください」）のではなく、理由を説明して提案する（ 「〜〜なので〜〜するとよいと考えています。いかがでしょうか」）ように、表現を和らげることがあります。少々回りくどいですが、同意できかねる部分があればそれを書きやすくなるのでは、と考えています。私が詳しくないシステムでは「ちょっと確認したいのですが」「間違えてるかもしれませんが」みたいな言葉を冒頭に入れてコメントを残すこともあります。私に限らず、教育のために、または、システムを知るために、詳しくない人がレビュワーに入ることもあります。その場合は、わからない部分を質問するのがやっとだということもあるでしょう。それでも、質問を繰り返していくと、互いの学びに繋がっていくと思います。表現は和らげて、お互いを尊重し、質問や反対意見も積極的に言えるようにしておきたいなと思っています。心理的安全性が高い状態、とも言い換えられるかもしれません。心理的安全性といえば、「心理的安全性」はなぜ混乱を招き続けるのか | Q by Livesense の記事もあります。ぜひご確認ください。
小さな CL | google-engineering-practices
小さなPRは、より早く、より完全に、バグを少なく、作業の無駄を減らし、マージしやすいです。レビュイーは、PRを小さくすることを心がけましょう。
30分でわかるFour Keysの基礎と重要性
30分でわかるFour Keysの基礎と重要性 - Speaker Deck
ハイパフォーマンスなチームは、変更リードタイムが短く、デプロイ頻度が高く、変更失敗率が低く、平均修復時間が短いです。 現在、これらの指標を計測できているわけではありませんが、レビューの質は変更失敗率に影響するでしょうし、デプロイ頻度、リードタイム、平均復旧時間には、PRの大きさやレビューのスピードが影響しそうです。
DevOps 測定: 仕掛り制限
DevOps 測定: 仕掛り制限 | Cloud アーキテクチャ センター | Google Cloud
処理量を増やすため複数のタスクに取り組むようにさせると、結果的に、タスクの完了に時間がかかります。そのため、「作業に優先順位を付ける」「作業する人を制限する」「少数の優先度の高いタスクを完了する」ことに焦点を当てるべき、という話です。レビューの際には必ず他人の作業の完了を待つ時間が発生します。その時間が長ければ長いほど、複数のタスクを並行して作業する必要がでます。並行作業を減らすため、他の人を待たせないようにする意識付けも重要だと思っています。
考え方
上述の話を我々のチーム向けにまとめ直したものを紹介します。以前は、スピード感に対する課題が大きかったので、その観点の話が多めです。チームの状況に応じて、コメントの付け方だったり別の観点も追加していくとよいのではないかと思います。
レビュイー
- PRは小さくしよう
- 大きなPRを作らないといけない前には、コンフル*1に設計を書き、レビューをしてもらおう
- Slackで相談、口頭で相談、設計資料をMTGでレビューなど、大きなPRを投げる前に、小さく確認しながら進めよう
- PRの「概要」に内容を端的にまとめて記載しよう*2
- PR内のコードに対して、自らコメントを付けて説明しよう
- コード中のコメントには残すべき情報を記載しよう
- PRのコメントには、習熟していない人向けの情報や、レビューを容易にするための情報などを記載しよう
- WIP(ドラフトPR)をうまく使おう
- レビューにより大きく変更が必要になったらWIP(ドラフト)に戻そう
- とりあえずドラフトPRを作って作業進捗や検証コードをチームに見せられるようにすることもおすすめします*3
- レビュワーからの一つ一つのコメントにできるだけ返答しよう
- レビュワーにレビュイーが返答し、レビュワーが同意・解決する、までがレビューです
- コミットだけを残されると、コミットからどう直ったかをレビュワーが判断する必要があり、レビュワーの負担が大きいです
- よい: レビュワー「〜〜を直してもらえませんか？」→レビュイー「〜〜（コミットID）で直しました！」→レビュワー「+:1」 →解決
レビュワー
- レビューはできるだけ早く対応しよう
- 最初の返事は、どれだけ遅くても、1営業日中（翌日朝方まで）が望ましいです
- Slack で、メンションをつけて話しかけられたときと同じように、レビューは放置せず、早くに応えてあげましょう
- レビュー完了もできるだけ早くしよう
- レビューの指摘は、細かく分けずにできるだけまとめて伝えてあげましょう
- よくない: 「ここ直して」→1日後→「ここ直して」→1日後→「ここ直して」→...
- よい: 「ここと、ここと、ここを直してもらえませんか。大きくコードが変わると思うので、もう一度またあとで見ます」
- レビューの指摘は、細かく分けずにできるだけまとめて伝えてあげましょう
- レビュイーから、Slackでレビュー依頼を受けずに始められるように、通知を設定しよう
- レビュイーがSlackでメンションを投げて「こちらのPRレビューお願いします」は機械化できる無駄な作業なので、できるだけなくしましょう
- Slack の GitHub Integrationの
subscribe機能、メール通知などの便利なツールを活用しましょう
- レビュイーから、催促を受けないようにしよう
- レビュイーの「レビューまだ終わってませんか？」 の確認は負担です
- チームのチャンネルでは、Slack の GitHubリマインダーを設定しているので、これに催促されないようにしましょう。また、これを無視しないようにしましょう
- dependabotのPRも人が作ったものとして扱おう*4
- テストが通らない、すぐにマージできないなどがあれば、issueを書いて、dependabotのPRはcloseしよう
おわりに
AIがあたりまえにコードを書くようになり、AIがレビューの一端を担うようにもなりつつあります。 しかし、人によるレビューはまだなくなりそうもなく、むしろ、人によるレビューの重要性がますます高まっているようにも思います。 ソフトウェアエンジニアリングにおいては、コミュニケーションも重要な要素です。一人でできる開発には限界があります。 品質や生産性、開発体験などの向上のため、レビューに限らず、よい振る舞いを明文化し、ときに説明できるようにしておくと、よいチームにつながると思います。なにか参考になれば幸いです。
