良いコードレビューは良いプルリクエストから！

はじめに

この記事はLITALICO Engineers Advent Calendar 2023 カレンダー2の12日目の記事です

LITALICOに2023年4月に入社した新卒エンジニアの@marie-yです。2022年からLITALICOで内定者インターン生として参加しており、レビューにとても難しさを感じていたので、昨年は内定者インターン生が贈る！初心者向けコードレビューの手引きという記事を書いていました。入社後、多くのレビューをしていく中で、良いレビューをするにはプルリクエストの作り方も重要なのでは？と感じたので、今回の記事ではプルリクエストの作成方法について、チームで使用しているプルリクエストのテンプレートを紹介しながら書いていきたいと思います。

想定読者

• プルリクエストの良い書き方がわからない新人エンジニア
• 新人エンジニアのハマりポイントを知りたいベテランエンジニア
• プルリクエストの書き方が定まっていないチーム

概要

この記事では、プルリクエストの良い書き方について、大きく以下の3つの点に分けて説明していきます。

• チームで使用しているプルリクエストのテンプレートの紹介
• 自動テストを書く
• セルフレビューをする

チームで使用しているプルリクエストのテンプレートの紹介

私のチームでは、以下のような項目が書かれたテンプレートを活用してプルリクエストのDescriptionを書いています。

• 関連Issue・PR
• 目的・背景・仮説
• 実装・変更
• レビューポイント
• 動作確認・テストケース
• ⚠️ ハマりやすい要注意ポイント
• スクリーンショット・ベンチマーク
• 今後の展望

これらの項目のうち、いくつかピックアップして説明していきます。

参考サイト：リポジトリ用のプルリクエストテンプレートの作成

目的・背景・仮説

目的・背景・仮説では、プルリクエストを作成する目的・背景・仮説を書きます。私は、コードを書くことに夢中になっているとついつい目的を忘れてしまいがちです。そのため、プルリクエスト作成時に改めて目的・背景・仮説を自分の言葉でまとめて、再認識しています。このとき、手段やプルリクエストの変更内容を書かないように気をつけています。しっかりと目的を冒頭で伝えて、レビュアーと認識を揃えておくことで、その目的ならもっとこうしたほうがいいのでは？という別の手段がレビュアーから出てくるかもしれません。

以下に悪い例と良い例を挙げておきます。
悪い例： フォームのボタンのデザインを変更するABテストを実施する ← 目的ではなく手段になっている
良い例：フォームの遷移率向上のために、フォームのボタンのデザインを変更するABテストを実施する

実装・変更

実装・変更では、目的を達成するために、プルリクエストでどんな実装をしたのか、どんな変更をしたのかを書いています。この項目は、概要と詳細の2つの観点があるので、2つの観点に分けて説明していきます。

概要

実装・変更の概要は、例えば、以下のようなことです。

例1）ログインしているユーザーに対して、トップページからマイページに遷移する導線を表示しました
例2）新たにYellowButtonというボタンコンポーネントを追加しました

上記の2つの例に挙げたように、この実装・変更の概要は一言でわかりやすく説明できることが重要です。もし、ここの説明で実装・変更が一言で説明できないようならば、プルリクエストの関心ごとが多くなってしまっている可能性があります。プルリクエストの関心ごとが多いと、レビュアーの負担が増えてしまい、抜け漏れ等のミスに繋がります。そのため、実装・変更の内容が多くなってしまった場合はプルリクエストを分割することを検討します。
参考サイト：「巨大プルリク1件vs細かいプルリク100件」問題を考える（翻訳）

詳細

実装・変更の詳細に関しては、Descriptionに記述をしてもいいですし、Files changedの方にコメントで残すのもおすすめです。Files changedに残すと、レビュアーがコードレビューをしている最中に目に入るので、コードレビュー時の助けになります。Descriptionに残すか、Files changedの方に残すかは判断が難しいですが、作成したメソッドの内容や変数の意味など、コードに書いていることを説明したいときはFiles changedの方に書いておいた方が、Files changedとDescriptionを行ったり来たりしなくていいので良いと思います。

レビューポイント

レビューポイントには、プルリクエストで特にレビューしてほしい点を書きます。例えば、以下のようなことです。

• テストで動作が保証できていそうか
• 削除すべきコードが削除できているか
• リファクタしているだけなので、既存の挙動に影響がないか
• 実装方針が良さそうか

レビュアーがプルリクエストについて隅から隅まで見るのは中々骨が折れます。レビューポイントを書いておくことで、そこに絞ってレビューができるので、レビューしやすくなります。

また、自分は新卒で自分の知識に自信がなかった点について細かめにレビューしてもらうために、以下のようなレビューポイントを残していました。

• テストコードの書き方に不安があるので、テストコードの書き方を細かめに見ていただきたいです
• Reactのemotionライブラリを使ってコンポーネントを作成することに不慣れなので、細かめにスタイルの当て方を見ていただきたいです

レビュアーは、どこまで新人が理解しているのかを把握しているわけではないので、自分の知識が不安な点は積極的に共有しておくと、レビュアーもどこまで細かく見るかの基準になるので、助かると思います。

動作確認・テストケース

動作確認・テストケースには、プルリクエストの変更内容の動作確認をする方法を書いておきます。レビューを始めたての頃は特にこの動作確認項目をまず確認するという手順を取っていたので、新人には特に助かる項目です。以下にこの項目の例を挙げます。私は、チェックボックスで列挙することが多いです。

• 非ログイン状態でトップページにマイページへの導線が表示されていないことを確認
• ユーザーとしてログインして、マイページへの導線が表示されていることを確認
• マイページに遷移できることを確認

この項目を書くときに、注意しておきたいことは、ページ名を書いた時は、URLのリンクも一緒に貼っておくことです。細かいですがページ名だけだと、どのページかわからなかったり、実装しているうちに自分で勝手に名前をつけていたりするので、レビュアーに確実にどのページかが伝わるように、URLも載せておくことをお勧めします。

スクリーンショット・ベンチマーク

スクリーンショット・ベンチマークには、フロント側の変更があった場合はその画面のスクリーンショットを書き、パフォーマンス改善などの変更があった場合はそのベンチマークを書きます。このとき、変更後だけでなく変更前も入れると後から見返したときにこの項目を見ればどこが変わったのがすぐわかるので良いです。私はよく、以下のように表形式で前後の比較を書くことが多いです。

Before	After
変更前のスクリーンショットなど	変更後のスクリーンショットなど

自動テストを書く

ここまで、プルリクエストの作成方法について説明してきました。次は、コードを書いている時に気をつけたいポイントについてです。コードを書いているとき、良いプルリクエストを作るために気をつけたいことは、自動テストを書くことです。自動テストを書くことの主な目的としては、作成した機能が正常に動作しているかを検証することだと思います。しかし、その副次的作用として、レビューの時間を短縮できるというメリットがあります。私がレビューを始めたての頃は、どんなプルリクエストでも動作確認までしっかりやることが多かったので、レビューに結構な時間がかかっていました。ですが、最近は軽微な変更のプルリクエストなら、テストを見て良さそうならApproveしています。もちろん、動作確認をすることも大事ですが、レビュアーも毎回動作確認をしているとかなり時間がかかってしまいます。そのため、テストが正しく書かれていて、そのテストが通っていて実装も問題なさそうなら、LGTMであるとして、Approveしても良いと思います。

動作確認の項目を自動テストでも担保する

私のチームには、テストを書く文化はあるのですが、どこまでテストを書けばいいかという基準が定まっていなかったので、私はどんな時にテストを書けばいいか分かっていませんでした。そんな時に、チームメンバーが心がけていることとして、以下のようなことがあることを知りました。

動作確認の項目に書いたことは機械的にも（大体）担保できるようにする

私は、動作確認の項目を丁寧に書く習慣があったので、動作確認の項目を基にテストを書くというやり方がしっくりきて、徐々にテストを書けるようになってきました。

動作確認の項目を基にテストを書く時の具体例を紹介します。例えば、以下のような動作確認項目を書いていたとします。

• ログイン時にトップページでマイページへの導線が表示されていることを確認
• 非ログイン時にトップページでマイページへの導線が表示されていないことを確認

この場合、ReactのReact Testing Libraryを使うと以下のように動作確認とほとんど同じ内容の自動テストを書くことができます。

describe('<TopPage />', () => {
  describe('非ログイン時', () => {
    it('マイページ導線が表示されていない', () => {
      render(<TopPage isLogin={false} />);
      expect(screen.queryByText('マイページ')).not.toBeInTheDocument();
    });
  });

  describe('ログイン時', () => {
    it('マイページ導線が表示されている', () => {
      render(<TopPage isLogin />);
      expect(screen.queryByText('マイページ')).toBeInTheDocument();
    });
  });
});

レビューに出す前にセルフレビューをする

最後に、良いプルリクエストを作成する上で、最も重要なことを述べます。それは、実装をしてプルリクエストを作成したら、それを全部忘れたつもりでセルフレビューすることです。自分のプルリクエストを他の人のプルリクエストだと思ってレビューしてみると、Descriptionやコードを読んだだけでは不足している、自分の頭の中の事前知識の部分がわかります。その事前知識の部分をDescriptionやFiles changedのコメントに書いておけば、レビュアーが読みやすいプルリクエストになるはずです。参考サイトに、私がセルフレビューの時に参考にしていた記事を載せておきます。
参考サイト：Pull Requestのセルフレビューでやっていること

レビュー指摘箇所の修正後にもセルフレビューする

細かいですが、自分が忘れがちなセルフレビューがあります。それは、レビューしてもらった箇所を修正して実装内容が変わった時、もう一度全体を通してセルフレビューを行うことです。これは、実装内容を修正したことによって、DescriptionやFiles changedに書いていたコメントと辻褄が合わなくなっている可能性があるからです。その指摘をしたレビュアーなら、文脈を把握しているので問題ないかもしれませんが、もし他のレビュアーが見た時や未来の誰かが遡ってプルリクエストを見た時など、全く文脈を把握していない人が見たときに、コードの内容とDescriptionの内容が異なっていると、余計な混乱を生んでしまいます。それをなくすためにも、実装を修正した後にはもう一度セルフレビューをして、辻褄のあっていないコメントを修正するようにします。

おわりに

今回は、良いレビューは良いプルリクエストから！ というテーマで自分のチームのプルリクエストのテンプレートや自分が重要だと思っているポイントを紹介してきました。書いていながら、自分もまだまだ整理ができていないなという点があったので、これからも日々良いプルリクエストの書き方を模索していきたいと思います
明日は、@s_ykさんの記事です。明日の記事もぜひ、読んでみてください