DroidKaigi2023で発表した「突撃!隣のコードレビュー 」のスライド資料
突撃!隣のコードレビューcreated by Okumura / Sato Taichi
View Slide
スピーカー紹介合同会社DMM.comAndroidエンジニアOkumura猫が好きSato Taichi 肉が好き
コードレビューは好きですか?
コードレビューのメリット 不具合が混入するリスクの軽減 可読性や保守性の向上 メンバーのスキル向上など
レビューに時間が 取られすぎる…
レビューに時間が 取られすぎる…ギスギスする…
レビューに時間が 取られすぎる…ギスギスする……など、課題も発生
・弊社の開発体制について・各チームの事例(3チーム) ・チーム概要 ・レビューの流れ ・チームで工夫していること ・チームで改善したいこと・個人の取り組み ・レビュアーとしての工夫 ・レビューイとしての工夫 ・Androidならではのレビュー観点話すこと(アジェンダ)
セッションの目標
セッションの目標コードレビューに対してほんのちょっと前向きな気持ちに
・弊社の開発体制について・各チームの事例(3チーム) ・チーム概要 ・レビューの流れ ・チームで工夫していること ・チームで改善したいこと・個人の取り組み ・レビュアーとしての工夫 ・レビューイとしての工夫 ・Androidならではのレビュー観点アジェンダ
DMMポイントクラブDMM TV DMMブックス開発チーム 開発チーム 開発チーム
どんなアプリを作っているか?DMMポイントクラブ→お得にさまざまなDMMのサービスを利用できるようにする、ポイントプラットフォームアプリ
チーム概要人数 5名開発の特徴・既存実装はAndroid Viewが多い・新規実装や改修時にはJetpack Composeを採用
レビューの流れプルリク作成CIチェックレビュー実施マージチェックリストに沿ってセルフチェックApproveがもらえるまで指摘→修正を繰り返すレビューしてくれた人全員からApproveをもらえたらマージ問題が 発生していたら対応
チームとして工夫していること実装前に行う工夫・実装前に設計のレビューで実装の流れを共有実装後に行う工夫・プルリクエストのテンプレートを工夫・チームとしてコードレビュータイムを設置
DMMポイントクラブのプルリクテンプレートissueのリンク対応の概要変更前後のスクショ
DMMポイントクラブのプルリクテンプレート設計レビューでもプルリクエストを作成セルフレビュー項目
セルフレビュー項目テスト項目書もここでセルフチェックDMMポイントクラブのプルリクテンプレート
トラッキングやパフォーマンス計測の実装についてもセルフチェック項目を用意DMMポイントクラブのプルリクテンプレート
コードレビュータイムについて元々は1回 30分のレビュータイムを1日に2回実施その時間で全員が集中的にレビューを行う②15:00〜15:30①11:00〜11:30勤務開始 勤務終了
それでもレビュー工数がかさみ、各メンバーの週の目標タスクが達成できない…課題コードレビュータイムについて
午前のコードレビュータイムで口頭でのコミュニケーションを実施するように(例)・タスクの状況・プルリクエストの内容を説明 ・タスクや実装についての困りごとを相談コードレビュータイムについて改善!
改善の結果…・日が浅いメンバーの相談ハードルが下がった・レビューを進めやすくなった・コミュニケーション取りやすくなったコードレビュータイムについて
これから改善したいこと模索 スプリント終盤に溜まるレビューの消化
スプリント終盤に溜まるレビューの消化 偏りがちなレビュアーの分散 チーム内におけるレビュー品質の均一化これから改善したいこと模索
どんなアプリを作っているか?DMM TV→オリジナルの番組や 漫画原作のドラマ、アニメが見放題のサービスを 利用できるアプリ
チーム概要人数 8名コードの特徴 KMP(Kotlin Multiplatform)で iOS/AndroidTVと一部を共有
レビューの流れプルリク作成CIチェックレビュー実施マージcode ownerの承認が得られたらマージ特にみてほしいところや対応issue、差分を確認するPFへの影響判断ラベルでautoassignCIで静的解析
チームとして工夫していること1プルリクの修正量 500行を目処ビルドチェック 行いたい時にコマンドをプルリク上でコメントすることで 各PFのビルドチェックを行う
レビューのリマインド 各PFのレビュー対象プルリクが多いため、 open状態のプルリクがある場合は 定期的にSlackでレビュアー向けにメンションを飛ばすチームとして工夫していること
DMM TVのプルリクテンプレートレビュー観点issueチケット
変更デザインプルリクで やったことプルリクで やってないことDMM TVのプルリクテンプレート
変更前後のスクショ影響範囲レビューイの 最終確認項目DMM TVのプルリクテンプレート
AndroidTV 確認項目動作確認項目iOS確認項目DMM TVのプルリクテンプレート
チームとして改善したいこと課題レビュアーの偏り特定分野の修正でレビュワーの偏りがある
チームとして改善したいことレビュアーの偏り特定分野の修正でレビュワーの偏りがあるレビューされるプルリクの偏り修正内容がパッと解らないプルリクは放置され気味課題
レビューされやすいプルリクを作るようにするを目標に 「差分を500行以下にする」や 「プルリクのスコープを狭める」などを行なっている。またプルリクに背景知識を書いておくなどを検討中。課題の解消改善中
その他CI/CD環境iOSのビルド等を考慮に入れてBitriseを選定。今後はコスパを考えて 定期的に調査することを視野に入れる。展望
チームC(ブックス)
DMMブックス電子書籍リーダー→DMMで購入した電子書籍を Android端末で閲覧するための 公式ビューアアプリどんなアプリを作っているか?
チーム概要人数 5名開発の特徴 アジャイル開発 ほぼKotlinでアーキテクチャはMVVM
開発の特徴 現在はリファクタリングが比較的多いフェーズ JetpackComposeへの移行を視野に入れているチーム概要
レビューの流れプルリク作成CIチェックレビュー実施マージ最低2人からApproveをもらえたらマージSlackでレビュー依頼を投げる。レビュアーはチーム全員チェックリストに沿ってセルフチェック。手元での動作確認中はDraftで作成問題が 発生していたら対応
チームとして工夫していることプルリクサイズの小型化 平均変更行数200行前後。 レビューにかかる時間が少なくて済む。 観点が限定されて見逃しがしにくい。
設計を重めに取る 設計段階で仮実装を利用して設計レビューを行い チーム内に相談して、レビュー観点や改修点を共有。 設計では全体のプロセス説明やクラス図、 変更前後のコードスニペットなどを記載する。チームとして工夫していること
チームとして工夫していることレビューで起きる問題コードレビューで問題が起きる場合、 実際には設計プロセスやコミュニケーションの不足などの レビューとは別の問題が、 コードレビューで露呈して起きると考えています。
レビュー工数の軽量化 要検討項目は資料で合意済みの状態を作った上で、 実装が資料通りの実装がなされているかを確認。 更にレビューイがプルリクにコメント記載して 意図を追記。チームとして工夫していること
DMMブックスのプルリクテンプレート対応の概要プルリクの 要点や観点
変更前後のスクショや動画対応の懸念点DMMブックスのプルリクテンプレート
レビュアーが 変更を確認するための方法レビューイの 最終確認項目DMMブックスのプルリクテンプレート
チームとして改善したいこと1. 現在少人数で回しているので、 人数が増えたらやり方が継続できるか不明瞭2. 動作確認の質を自動化などで均一化できておらず 工数もそれなりにある課題
QAチームと共同でシナリオテストを作成中。各画面の確認すべきことを文書化して、動作確認の際に品質を安定化させたい。レビュー品質の均一化改善中
DMMポイントクラブ DMM TV DMMブックス取り組みについての小まとめ
チームごとの取り組みについての小まとめ工夫プルリクは小さく作る プルリクの変更行数を出来るだけ少なくして、 見やすいものにするCIをしっかり利用 レビュー前にCIを利用して細かいミスは排除する
チームごとの取り組みについての小まとめ課題レビュアーの偏り レビュー対応するレビュアーが偏ってしまうレビュー品質の偏り チーム内でのレビュー精度に偏りが出て 品質が均一化出来ていない
チームごとの取り組みについての小まとめ展望メンバーを教育 実装やアーキテクチャの思想を 設計やレビューを通して、 メンバー全員が共通の認識を持てるようにしていく
レビュアーとしての工夫
よく挙がった観点・認識をきちんと合わせる・プルリクを記録やナレッジとして活用できるようにする・スムーズなレビューを目指す・心地よいコミュニケーションを心がけるレビュアー(レビューする側)としての工夫
レビュアー(レビューする側)としての工夫認識をきちんと合わせる
レビュアー(レビューする側)としての工夫認識をきちんと合わせる・ちょっとした疑問でも聞く
レビュアー(レビューする側)としての工夫認識をきちんと合わせる・ちょっとした疑問でも聞く・プルリク上でのやりとりが3往復以上になりそうな場合は、 対面レビューやモブレビューなどを実施し早期解決に努める
レビュアー(レビューする側)としての工夫認識をきちんと合わせる・ちょっとした疑問でも聞く・プルリク上でのやりとりが3往復以上になりそうな場合は、 対面レビューやモブレビューなどを実施し早期解決に努める・コメントをした背景や理由をより具体的に明記しておく
レビュアー(レビューする側)としての工夫記録やナレッジとしてプルリクを活用
レビュアー(レビューする側)としての工夫記録やナレッジとしてプルリクを活用・口頭で確認したこともプルリクのコメントで残す
レビュアー(レビューする側)としての工夫記録やナレッジとしてプルリクを活用・口頭で確認したこともプルリクのコメントで残す・指摘に対して修正コミットを入れるだけではなく、 返信・Resolveされているかまで確認する
レビュアー(レビューする側)としての工夫記録やナレッジとしてプルリクを活用・口頭で確認したこともプルリクのコメントで残す・指摘に対して修正コミットを入れるだけではなく、 返信・Resolveされているかまで確認する・レビューコメントを読んだ人が今後のレビューをする・ されるときに役立つように詳細に書く
レビュアー(レビューする側)としての工夫スムーズなレビューを目指す
レビュアー(レビューする側)としての工夫スムーズなレビューを目指す・自分のタスク < コードレビュー
レビュアー(レビューする側)としての工夫スムーズなレビューを目指す・自分のタスク < コードレビュー・なるべくすぐ見る
(人やチームによって方針は要検討)}レビュアー(レビューする側)としての工夫スムーズなレビューを目指す・自分のタスク < コードレビュー・なるべくすぐ見る・指摘の優先度がわかりやすいように頭に分類をつけている →例:[MUST][IMO][nits]
レビュアー(レビューする側)としての工夫心地よいコミュニケーションを心がける
レビュアー(レビューする側)としての工夫心地よいコミュニケーションを心がける・固すぎずゆるい感じの文体でコメントする
レビュアー(レビューする側)としての工夫心地よいコミュニケーションを心がける・固すぎずゆるい感じの文体でコメントする・柔らかい口調にする
レビュアー(レビューする側)としての工夫心地よいコミュニケーションを心がける・固すぎずゆるい感じの文体でコメントする・柔らかい口調にする・自分の書き方の好みを押し付けない
レビュアー(レビューする側)としての工夫心地よいコミュニケーションを心がける・固すぎずゆるい感じの文体でコメントする・柔らかい口調・自分の書き方の好みを押し付けない・指摘だけではなく、良いと思ったところもコメントする
レビュアー(レビューする側)としての工夫その他
レビュアー(レビューする側)としての工夫その他・レビューしたコードを他の人にも説明できるくらいに責任を持つ
レビュアー(レビューする側)としての工夫その他・レビューしたコードを他の人にも説明できるくらいに責任を持つ・自分が実装するならどうするかというのを意識して、 違っている点を質問・コメントして自己学習やコード改善につなげている
レビューイとしての工夫
差分のサイズを小さくするレビューイ(レビューされる側)としての工夫
差分のサイズを小さくする・プルリクの粒度が大きくなりそうな場合、 「テストは別プルリクで作成」など申し送り事項を設定しスコープを狭めるレビューイ(レビューされる側)としての工夫
差分のサイズを小さくする・プルリクの粒度が大きくなりそうな場合、 「テストは別プルリクで作成」など申し送り事項を設定しスコープを狭める・一瞬でわかるプルリクが理想なので、 プルリクのスコープや影響範囲を狭くして、 本筋以外の修正は含めないレビューイ(レビューされる側)としての工夫
差分のサイズを小さくできなくても・・・レビューイ(レビューされる側)としての工夫
差分のサイズを小さくできなくても・・・・修正量が多い時はコミットを分けて、 コミットを追うことで修正内容がわかりやすいようにするレビューイ(レビューされる側)としての工夫
差分のサイズを小さくできなくても・・・・修正量が多い時はコミットを分けて、 コミットを追うことで修正内容がわかりやすいようにする・どうしても大きくならざるを得ないプルリクは、 本当に見てほしいところをコメントでピックアップするレビューイ(レビューされる側)としての工夫
読みやすいプルリクにするレビューイ(レビューされる側)としての工夫
読みやすいプルリクにする・意味のあるまとまりでプルリクを作成する (ドメイン層などのレイヤーごと・画面ごと・機能ごとなど)レビューイ(レビューされる側)としての工夫
読みやすいプルリクにする・意味のあるまとまりでプルリクを作成する (ドメイン層などのレイヤーごと・画面ごと・機能ごとなど)・処理の意図をコードコメントだけでなく、 プルリクコメントでより具体的に補足するレビューイ(レビューされる側)としての工夫
必要な情報をきちんと揃えるレビューイ(レビューされる側)としての工夫
必要な情報をきちんと揃える・仕様や設計書のリンクなど、レビューに必要な材料を揃えてプルリクに書くレビューイ(レビューされる側)としての工夫
必要な情報をきちんと揃える・仕様や設計書のリンクなど、レビューに必要な材料を揃えてプルリクに書く・レビュアーが見たときに、 合っている・合っていないの判断ができるように情報を整えるレビューイ(レビューされる側)としての工夫
必要な情報をきちんと揃える・仕様や設計書のリンクなど、レビューに必要な材料を揃えてプルリクに書く・レビュアーが見たときに、 合っている・合っていないの判断ができるように情報を整える・プルリク上で動作確認のエビデンスとして動画を添付しているレビューイ(レビューされる側)としての工夫
その他・指摘を修正後、コミットのリンクを付けて返信 (指摘ごとにコミットを分ける)レビューイ(レビューされる側)としての工夫
・差分のサイズ・コミットを分割・意味のあるまとまり・コメントで補足・必要な情報を揃えるetc…レビューイ(レビューされる側)としての工夫
レビューイ(レビューされる側)としての工夫・差分のサイズ・コミットを分割・意味のあるまとまり・コメントで補足・必要な情報を揃えるetc…集まった意見はすべて「いかにレビューしやすくするか」にフォーカスしたもの
レビューはコミュニケーションと思いやりめっちゃ大事
Androidならではのレビュー観点
iOSと比較して端末が多種多様(性能も画面サイズも)→パフォーマンス面、UI面の考慮が必要Androidならではのレビュー観点
パフォーマンスについて ・メインセーフなつくりになっているか ・メモリリークを起こすところが無いか (LeakCanaryなどのツールを使う) ・レイアウトの入れ子構造が深くないかAndroidならではのレビュー観点
Androidならではのレビュー観点UIについて ・画面が小さい端末や、逆に大きい端末での表示が 考慮できているか ・文字サイズや画面ズームの設定を変更しても崩れないか
その他 ・Activity破棄時・再生成時の挙動を考慮できているか ・OSバージョン間での挙動に問題がないかAndroidならではのレビュー観点
〜アジェンダここまで〜
セッションの目標の振り返りコードレビューに対してほんのちょっと前向きな気持ちに
Special Thanks・インタビューに応じてくださった皆さん・このスライドをレビューしてくださった皆さん・セッションを聴いてくださった皆さんありがとうございました!
最終ページ