株式会社dinii の whatasoda です。今年9月に新卒入社から2年半ほど勤めたメルカリを退職し(インターン含めると計3年ほど、大変お世話になりました！！！)、10月から dinii にジョインしました。（ここまで退職・入社エントリ）
今回は Qiita史上最多記録をつくろう！アウトプットはいいぞカレンダー Advent Calendar 2022 の カレンダー3 の初日にエントリーさせていただいております。
さて、 dinii では2022年12月現在フルタイム7名、業務委託5〜10名ほどの少人数エンジニアチームで10個近いプロダクトを開発しています。使用言語を TypeScript に統一しているためプロダクト間でのコンテキストスイッチコストはかなり低いものになっているものの、スピーディーな価値提供のためにアジリティの高い開発を続けるためには個人としてもチームとしても開発効率というものを改善する取り組みをしていく必要があります。
ジョインして数週間経ったときにこういった内容についてチーム内で話す機会があり、開発効率向上のためにそれぞれがやっていることを共有する場を設けることになりました。（社内ではこれをワークショップと呼んでいますが、時間の都合もありほとんど一方的にしゃべるだけなので実質セミナーになっています😇）
これまでに私が2度開催しており、第一回は『開発環境効率化 VSCode編』、第二回は『レビューしやすい Pull Request を目指して』という内容でした。今回は「第二回で取り上げた内容をまるっと記事にしてしまえばアドベントカレンダーに出せるのでは？」という最高のアドバイスをチームメンバーに頂いたので、そのアイデアにそのまま乗っかって『レビューしやすい Pull Request を目指す』ために抑えておきたいと個人的に考えているポイントについて共有します。
Pull Request も一種のアウトプットですよね！(アドベントカレンダー意識)
チーム開発ではコードレビューがとても重要で、可能な限り時間をかけるべきだと考えていますが、一方でアジリティの高い開発のためには出来るだけ短い時間で同じ品質のレビューをできるのが理想です。この改善にはレビュアー側の努力や工夫以上に、PR 作者の努力や工夫が大きな影響を与えます。
以下、実際のワークショップで共有した資料です。
Goal
- Pull Request (以下 PR) のレビューを依頼する前に意識しておきたいポイントをチーム内で共有する
Non-Goal
- PR の作り方に特別なルールを設ける
登場人物
Author
PR の作者。その PR のことを一番よく理解している人
Reviewer
プロダクトやコードの品質を担保するため、PR に対するフィードバックをする人
基本の考え方
Reviewer がコードをレビューするときに一番コストがかかるのは、その PR のコンテキストを理解するというステップになります。これは最初に必要なステップで、どんなに変更の少ない PR でも「なぜその変更をするのか」ということを理解していなければ正しくレビューをすることはできません。大抵の場合、以下の点が分かればこれをある程度理解することができます。
- その変更をする前はどうなっていたのか
- その状態の何が問題だったのか
- その変更をした後はどうなるのか（どうなることが期待されているのか）
また、変更の規模や内容によっては 「なぜその方法で変更したのか」という情報がレビューする上で重要になることもあります。
- その変更方法の利点と欠点
- 他に解決方法はあるのか
- ある場合、なぜ採用しなかったのか
こうした情報を得るために Reviewer はチケットや PR の説明などをまず確認します。そこで十分に情報が得られなかった場合はコードの差分を見てそれを推測したり読み取ったりする必要があります。
一方で Author はそれらの点について一番よく理解しています。一人の Author がちょっと時間をかけてそれらの情報を適切に共有するだけで、複数の Reviewer 達がその情報のキャッチアップにかけなくてはならない時間を大幅に減らすことができます。そうして節約できた時間で Reviewer 達は自分のタスクや他の PR のレビューをすることができるため、レビューしやすい PR をつくることを心がけることはチーム全体の生産性の向上に繋がります。
PRにのせたい情報
上記の情報を共有してレビューしやすい PR にするために抑えておきたい情報をリストアップしています。
変更後の動作確認ができる動画や画像
UI の変更が含まれている場合などは、視覚的に動作の確認ができる変更の場合は動画や画像があるとわかりやすい。長い動画の場合は ffmpeg などで圧縮すると GitHub の容量制限を回避できる。
自分が Reviewer なら指摘しそうな部分についての注釈
こういった点に予め気づくことができたなら、先にコメントを残しておくといい。
例えば既存のコードと少し違うやり方をしなくてはならないときにはその理由を載せておくと Reviewer が「なぜその方法で変更したのか」という質問から入らなくて済む。
PR の主旨に関係のない変更についての注釈
特にプロダクトの動作の変更を伴うものについては予めコメントをのこしておく。小さい変更ほど他の変更に紛れてしまいがちなので気をつける。
PdM や Designer などとのコミュニケーションの記録
実装前や実装途中で仕様やデザインを詰めた場合、それらもその PR のコンテキストの一部であるため、記録へのリンクなどがあると Reviewer のコストが減らせる。
実装の段階で気をつけること
できるだけ既存の実装でやっている方法に揃える
実装の方法について細かく見るコストが下がるため
変更を小さいコミットに分ける
PR 全体の差分だと確認コストが高いがコミットごとで見るとコストが低い場合がある。そういった場合には実装の段階から小さいコミットに分けておいて、 Reviewer にコミットごとに確認してもらうよう依頼すると効率的なことがある。小さいコミットに分かれていなくても頑張ればレビューはできるので優先度は低い。
似た変更をまとめる方法や、適宜こまめにコミットして時系列的に分割する方法など、分け方には色々ある。
コミットログをちゃんと残す
そのコミットが何をするためのものなのか、特にレビュー対応のコミットについてはどのレビューに対する対応なのかがわかるようなコミットメッセージをつけられると良い。
wip だけや fix: update based on feedback などは避ける。
その他
セルフレビューをする
Reviewer にレビューリクエストを出す前、Draft で PR を作った段階で一度自分で軽くレビューをしてみると、消し忘れた console.log や検証時に使った一時的なコードなどがあったときに気がつける。また、まず Draft で PR を作っておくと CI が失敗しているときにレビューリクエスト前に気づくことができる。
セルフレビューをすることで、 Reviewer がレビューをするときにノイズとなるような情報に予め対処しておくことができる。
レビュー対応
理想は 1 フィードバックに対して 1 コミットで対応していくことだが、レビュアーが確認しにくくなければまとめてしまっても構わない。逆に細かすぎると分かりにくいこともあるため、適切な粒度でレビュー対応のコミットをする。
merge か rebase か
ターゲットブランチとのコンフリクトがあった場合、merge と rebase の2つの解消方法がある。
最初のフィードバックを受ける前はどちらでも構わないが、一度レビューを受けたら rebase は避けたい。rebase だと force-push になるのでコンフリクト解消時の差分を確認しづらくなってしまう。
merge だと merge commit が入るが、最後は squash-merge をするので commit log のきれいさは気にしなくても大丈夫。
コードコメントか PR コメントか
コードコメント
- ビジネスロジックに関連するもの
- ぱっと見て意図がつかみにくいもの
- TODO, FIXME
PR コメント
- なんでも
PR コメントは後からコードコメントにしやすいので迷ったらとりあえず PR コメントにして、 Reviewer とコードコメントにするべきか相談するのもアリ。何もコメントしないのが一番怖い
以上、本当にワークショップで使った資料をほとんどそのまま貼り付けただけになりますが、『レビューしやすい Pull Request を目指して』というお話でした。
dinii では各プロダクトに1〜2名いるコードオーナーが主にコードレビューを担当しています。特に自分がジョインしたときは1ヶ月前に2名のエンジニアが新しくジョインしたばかりで（その2名についてはこちらの記事で紹介されています！）、フルタイムエンジニアが4人からほぼ2倍の7人に増えたというタイミングでした。そのため、コードレビューが必要な PR の数も一気に増えたのではないかと思います。
まだまだチームの人数が少ない段階ではありますが、このタイミングから真心のこもった Pull Request を作れるエンジニアチームを目指しておくことで、今後一緒にプロダクトをつくっていくエンジニアがもっと増えたときにもコードレビューで疲弊せずにアジリティ高く開発していけるような開発組織になれるのではないかと考えています。
Discussion