JMDC TECH BLOG

JMDCのエンジニアブログです

トップ>GitHub>Pull Requestの質を向上させるために行った戦略/戦術の話

2022-09-09

Pull Requestの質を向上させるために行った戦略/戦術の話

株式会社JMDCでモバイルアプリエンジニアをやっている@mrtry です。入社した当初、モバイルアプリチームのエンジニアは私一人だったのですが、現在では4人になりました。最近はPull Requestのレビュー数も爆増しており、とても疲弊しがちです(嬉しい悲鳴)。たいへんポイントを減らすために、最近Pull Requestまわりの運用を整えたので、今日はその話をしたいと思います。

Pull Requestのレビューがたいへん

現在、モバイルアプリチームでは、3つのプロダクトの開発をしています。各プロダクトに1名ずつassignされており、リードエンジニアとして私が一通りレビューをしている状況です。そんなこともあり「Pull Requestのレビューがたいへん」というのが最近の悩みでした。

Pull Requestのレビューをするとき、私は以下のような観点でレビューしています。

機能仕様レベルで、求めるユースケースと解決となる機能実装の対応が妥当か
技術仕様レベルで、機能実装の実現方針が妥当か
コードレベルで、変更内容が妥当か

変更されたコード全体を結合して見たとき、機能要件を満たしているか
変更内容をcommit単位で見たとき、各commit内容が適切な意味を成しているか
変更内容に依って出る影響範囲が考慮されているか

上記の観点でレビューする際に、以下のような指摘をすることがありました。

機能仕様レベル

動作確認方法がわからない
ひとつのPull Requestで扱うcontextが大きすぎる
ひとつのPull Requestで扱うcontextが複数ある

技術仕様レベル

実装方針が適切ではない場合、全体的に作り直しになることがある

コードレベル

ひとつのPull Requestでコードの差分が大きい
commit messageがcommit内容の説明になっていない
各commitの粒度が大きく、commit messageの1文で説明が足りない
改修による影響範囲の想定がわからない

これらの指摘内容が含まれているPull Requestのレビューをすると、改修の妥当性の判断が難しかったり、前提理解のために時間がかかったりすることが発生し、レビューの効率や質が悪くなってしまいます。

なるべく効率的に、また効果的なPull Requestのレビューをできるようにするために、取り組んだことを紹介します。

やったこと

Pull Requestのテンプレートをつくった

GitHubにPull Requestのテンプレートをつくりました。必要な情報を予めテンプレートとして列挙しておくことで、Reviewer視点ではコードを読む前に必要な情報をインプットしやすくなり、Reviewee視点では自身の作業内容についてセルフレビューすることができます。

これだけ見ると各項目に何を記入するのかわかりにくいので、テンプレートにコメントを入れてあります。また、チームメンバーに日本語が母国語でない方もいるので、英語も併記してあります (ただ、翻訳を通して雑にいれたものなので、英文的な正しさが微妙かもしれません)。

実際に作成したテンプレートは以下になります。こちらのPull Requestテンプレートをベースにしています。

Pull Request のフォーマットを決めるとレビューの効率が3倍よくなる :: Crocos Engineering Blog

<!--  [Pull Request title]  他人に伝わるように変更内容を1行でまとめる  また、エンジニア以外の職種の人にも理解できるようなタイトルにする  Summarize the changes in one line so that others can understand them.  Also, make sure the title is understandable to non-engineers--># 概要 / Overview<!--  Pull Requestについての概要  エンジニア以外の職種の人が読んでも理解できるよう、機能仕様をメインに書く  家電の説明書のように、使い方やユースケース、変更内容を説明する  電気回路の設計のような、エンジニア以外に伝わらない技術的なことは書かない  Overview of Pull Requests  Write mainly functional specifications so that non-engineers can read and understand them.  Explain usage, use cases, and changes, like an instruction manual for a household appliance.  Do not write about technical matters that cannot be understood by non-engineers, such as the design of electrical circuits.-->## Issue (Jira)<!--  Jiraのリンクを貼る  リンクを複数列挙するときは、PRを複数に分割することを検討する  複数の機能実装をまとめてレビューすると、レビューの精度が落ちたり、時間がかかる可能性がある  Link to jira.  When enumerating more than one link, consider splitting the PR into several.  This may reduce the accuracy of the review and may take more time.--># Screenshot
<!--  画面の新規実装や改修があった場合は、スクリーンショットや動画を貼る  If there are any new screen implementations or modifications, post a screenshot or video.-->| before | after || --- | --- ||  |  |# 技術的な変更点 / Technical changes<!--  実装した内容について、具体的な変更点を書く  内容は技術仕様をメインとし、同僚のエンジニアに引き継ぎをする気持ちで書く  Write about the specific changes you have made to your implementation.  The content should be mainly technical specifications, written with the feeling of handing over to your fellow engineers.--># 機能テスト / Feature test<!--  実装した機能について、テストをした内容を書く  レビュアーが実際にテストできるように一文で書く  Unit testやStorybookなど、コードレベルで動作保証したものについてはここに書かなくても良い  テスト観点  - ユースケースシナリオに則った動作確認    - ex) XXXという操作をしたら、画面がXXXという状態になる  - 状態遷移テスト      - ex) ログイン時、XXXが表示される      - ex) 未ログイン時、XXXが表示される      - ex) データが0件のとき、XXXが表示される      - ex) ネットワークエラーのき、XXXが表示される  Write a description of the functionality you have implemented and tested.  Write it in one sentence so that reviewers can actually test it.  Do not have to include a description of any code-level assurances, such as unit tests or storybooks.  - Confirm the behavior in accordance with the use case scenario    - ex) If XXX is done, XXX will happen.  - State transition test    - ex) When logging in, XXX is displayed.    - ex) When not logged in, XXX is displayed    - ex) XXX is displayed when there is no data    - ex) XXX is displayed when there is a network error.--># その他 / Others<!--  レビュワーへの申し送り、重点的に見てほしいところ、などあれば書く  Write down any remarks to reviewers, areas you would like them to focus on, etc.-->