iOSチームレビュー体制(ルール/目的/観点/よくある質問) まとめ (2021/12)
これはand factory.inc Advent Calendar 2021の記事です。
はじめに
iOSチームのレビュー体制(ルール)/目的/観点/よくある質問 に関しての記事です。
全体的に現状のiOSチームのレビュー体制をもとに話を進めています。
実際にどういったルールでレビューを運用しているのかを記載しています。
個人の観点の記事はみかけますがチームとしてはあまりないのでチームとしての観点 + ちょい個人的な観点のまとめになります。
最初は現状の体制だけを書こうと思ったのですが、目的/観点/よくある質問 がないと気になる点が多いなと思ったので追加しました。
前提
チームの人数: 約10~15人
iOSアプリの個数: 約8~10個
1アプリのiOSエンジニア担当人数: 約1~2人
レビュワーもレビュイーも全員iOSエンジニア
運用年数: 約1年~2年
※合計で4年ぐらい運用していますが、安定したのは1,2年ぐらいです
体制(ルール)
- 1アプリに対してのレビュワーは約5~6人
- 自分のアプリのレビュー(自分が出したPRを除く) + 担当のアプリのレビューを基本的には全部担当
- 自分のアプリ + 2~3アプリぐらいが担当
- レビュワーが全員Approveする必要はない
- できれば１人はApproveしてほしい
- 24時間経ってもApproveされなかったらマージしてい良い
- 急いでいる場合は即セルフマージしてOK
- PRはセルフマージ可能
- 基本的にはレビューを経てからマージする
- xib, storyboard のみの修正はセルフマージ
- その他特に見るところがないPR(PRを出した人が判断)もセルフマージ
- 半期に1度レビュー担当を変更
- とくにルールはなく雑にメンバーをシャッフル
- 一応エンジニアのレベル感が各アプリのレビュワー全体で揃うように調整
レビュー目的
優先度高い順
- メンバーの教育/成長のため
- 他人のコードを読む(シニア⇔ジュニア相互)ことでコードの幅を広げてほしい
- 属人性排除のため
- 自分がわかるコードではなく、他人がわかるコードを目指してほしい
- ソフトウェアの品質を高める/均一化するため
- Swift/iOS的な観点、アーキテクチャー的な観点、アルゴリズム的な観点等
- 高めるというよりはほかアプリでやっている知見を共有できれば良いなぐらいの感覚
- バグなどの問題の混入を防ぐため
- アプリをまたぐとこの辺はあんまりわからないので重視していません
- できたらラッキー的感覚
レビュー観点
優先度高い順
※もっと細かいところをあげようと思ったらもっとあげれますが省略しています
- 他人のコードを読む(シニア⇔ジュニア相互)ことでコードの幅を広げてほしい
- 自分のコードを他人に伝わりやすくするために思考する
- 他人のコードを理解できるようにするために思考する
- 質問し相互理解を深める
- 意図が伝わるコードが書けているかどうか
- 採用しているアーキテクチャーに則って実装ができているかどうか
- 該当クラスの責任範囲かどうか
- ifかguardか
- indexやtagをできるだけ使わずにenum等に変換しているか
- 継承するべきか
- Protocolにすべきか
- 適切にコメントが書けているかどうか
- マジックナンバー
- 特定OS、特定画面サイズの場合の処理
- 変数名がわかりやすいか適切か
- Swift/iOS的に問題がないか、適切な実装かどうか
- Crashしないかどうか
- メモリリークしないかどうか
- アルゴリズム的により良くかけるかどうか
よくある質問
1.ルールがゆるくないですか？
性善説運用なのでこのぐらいにしています。ルールというよりは仕組みみたいなものかもしれませんね。
2.レビューしない人はどうしますか？
基本的に全員レビューするので現状問題としていません。
もし、レビューしない人が出てきても1アプリに約5~6人レビュワーがいるので誰かがレビューすれば問題ない認識です。
3.レビューが多いと負担になりませんか？
レビューを全部見る必要はなく、忙しいときは見なくてもマージされていくので問題ない認識です。
また、現状そこまでレビューできないという声を聞いていないので現状問題としていません。
4.セルフマージを許すとレビューがスキップされて健全じゃなくなりませんか？
一時期1人以上を必須にしていましたが、逆に急いでいるときもレビューしなくてはいけない負担の方が問題として大きかったので、必須化をやめました。
5.他のアプリをレビューする際にドメイン知識がないとレビューできなくないですか？
メンバーの教育/成長のため、属人性排除のためを主目的としているため、ロジック的に正しいかどうか等をレビューする必要性はないので、ドメイン知識がなくてもそこまで問題では無いと思っています。
また、半年ほど同じアプリのレビューをしているとある程度ドメイン知識が身についてくるので、ある程度長期間同じアプリをレビューするようにしています。
6. 半年に1度レビュー担当を変更するのはなぜですか？
メンバーの教育/成長のため、属人性排除のためを主目的としているため、同じメンバーで長期的に固定せずに定期的にメンバーを入れ替えています。期間が半年がベストなのかはわかりませんがある程度固定にするメリットと入れ替えるメリットの両方を受けることができるので悪くない期間が設定できているのかなと感じています。
その他レビュー体制構築で大事だったなと思うこと
DDDをチームで1冊読んだらレビューがめちゃくちゃよくなった
コードに意図が見られるようになり、レビューするときに読みやすく/指摘しやすくなったと思います。
とくに、ユビキタス言語とレイヤードアーキテクチャの知見がチーム内で共有されたことによってレビューの快適さにつながったと実感しています。
以下の本をチーム内で輪読会しました。
ドメイン駆動設計 モデリング/実装ガイド - little-hands - BOOTH
https://little-hands.booth.pm/items/1835632
大きすぎる指摘は勉強会で共有する
そもそもMVVMがわかっていないさそうとか、プログラミングに関する法則・原則、デザインパターンの話、凝集度/結合度の話とか。ある程度の規模で話ができそうなものはレビューではなく勉強会などの場で共有する。共有した上で実際のコードレビューで指摘する。そうしないとレビューで無限に指摘することになり、PRがマージされるのを阻害してしまうので、そもそもの考え方等はレビューの機会とは別で行うようにしています。
Swiftlint導入
導入しています。導入するときに個別の項目に対して共通認識を持てるのが良い点かなと思っています。全チームでそろっているわけではないですが、細かいところはSwiftlintに怒られていないなら良いかと思ってスルーできるのでいいですね。
参考までにデータ
Pull Reminder の 過去14d の Leaderboard からデータを引っ張ってきています。
平均PRレビュー数: 14.09
平均PRレビュー速度: 6.45hrs
おわりに
最初は自分が担当しているアプリ以外のレビューに前向きではなかったですが、1~2年気にせず運用していたらもはや当たり前になりつつあります。
レビュー担当性も、PRごとに割り当てるよりも、アプリ単位で割り当てたほうが継続的にレビューできるので低コストでレビューの効果を得やすいのかなとも思います。
上記の点などを１~２年模索した結果今の形になり約2年ぐらいは運用していますが、現状いい感じに回っているのかなと行った印象です。(改善点ありそうだったらご連絡いただけると最高にハッピーです)
Discussion