Zenn
🐱

GitHubのコードレビューを受ける際に気をつける事

2020/12/06に公開
Git
GitHub
idea

title

はじめに

READYFOR に業務委託として参加している、フリーランスエンジニアの keitakn です。

この記事は READYFOR Advent Calendar 2020 の6日目の記事です。

概要

GitHubを用いたコードレビューを受ける際に私が気をつけていることを共有します。

対象読者

以下の方々を対象としています。

  • Gitの基本操作
  • GitHubの基本操作を理解している人
  • GitHubのプルリクエスト（PR）機能をある程度使ったことがある人
  • GitHubを用いたチーム開発をしている人

筆者のバックグラウンド

エンジニア歴はもうすぐ8年程です。

過去には6〜7名の小さなチームの スクラムマスター やチームリーダーの経験があります。

現在はフリーランスとして活動していますが READYFOR株式会社 さんを始め、何社かの自社開発企業さんでコードレビューをしています。

私の書いたコードをレビューしていただくというケースも多々あります。

レビューを受ける人の事を レビューイ (reviewee)、レビューを行なう人を レビュアー（reviewer） と言いますが、私はどちらの立場も理解していると自負しています。

この記事は レビューイ (reviewee) 視点です。

この記事で説明すること、説明しないこと

説明すること

タイトルの通りですが、 私がGitHubでコードレビューを受ける際に気をつけていることを解説します。

説明しないこと

  • GitHubの基本的な使い方
  • Gitの基本操作

コードレビューを受ける際に気をつけること

主にプルリクエスト（以下はPRと表記）の出し方になります。

下記にサンプル用のPRを作ってあります。

https://github.com/keitakn/go-cognito-lambda/pull/27

各項目に関しては、個別で説明します。

PRにはわかりやすいタイトルをつける

PRのタイトルは何を修正したかわかりやすいタイトルをつけましょう。

Defaultだと以下のように最終コミットメッセージが自動で挿入されます。

PRTitle

これをそのままPRのタイトルとして利用しているケースをたまに見かけます。

コミットメッセージがそのままPRのタイトルとして適切なら良いですが、そういうケースのほうが稀でしょう。

あとで検索しようとした際に検索しても見つけることは難しいので、必ず立ち止まってわかりやすいタイトルを考えましょう。

PRの説明欄に書くべき事

少なくとも以下の情報を説明欄に記載します。

.github/PULL_REQUEST_TEMPLATE.md というファイルをコミットしておくとTemplate化できるので、オススメです。

GitHub issueのURL（または課題管理ツールのチケットURL）

チーム開発の場合何らかの課題管理ツールを使っている場合が多いと考えられます。

課題管理ツールのチケットURLを記載しましょう。

目的は、このPRがどの課題を解決しようとしているのかを明示的にわかるようにするためです。

私は現在、どの現場でもGitHub issueを使っているのでGitHub IssueのURLを記載するようにしています。

このPRの対応範囲

このPRで対応する範囲を明確にしましょう。

課題管理ツールのチケットURLを記載しているので、どの課題を解決しようとしているのかはわかります。

しかし課題によっては一度のPRで解決することが難しい場合もあります。

そのためにも必ず、PRの対応範囲を記載します。

これが明記されていないとレビュアー側から今回対応していなくてあとで対応するのか、それとも対応を忘れているのか判断が付きません。

// 例
# このPRの対応範囲
ここでは単純にメールアドレスを登録するところまでの実装となります。

「RFC違反のメールアドレスは登録出来ないようにする」という要件は別のPRで対応します。

一度のPRで修正できる、小さいissueであれば以下のように記載しても構いません。

# このPRの対応範囲
https://github.com/keitakn/go-cognito-lambda/issues/26 を解決します

変更内容＆変更理由

問題を解決するために、具体的に何を変更したのかを記載します。

変更内容に関しては差分を見ればわかる部分も多いですが、どのような意図で変更したかは変更した本人にしかわかりません。

問題解決の手段が限られている場合、変更理由を書くのが難しいときもあります。

その場合は変更内容を具体的に記載するだけで良いですが、変更に特別な意図がある場合は必ず記載しましょう。

レビュアーに重点的にチェックして欲しい点

中心的に見て欲しい箇所や観点があれば書いておきましょう。

その他の情報

必要に応じて、以下の情報を追記します。

  • 関連URL（修正箇所のURL、ネイティブアプリなら対象となる画面）
  • スクリーンショット（デザイン変更が入る場合は添付したほうが良い）

PRは適切な大きさに分割する

変更箇所が大きなPRはレビュアー側の見る負担が大きいので、小さい単位に分割します。

具体的な例をあげて説明します。

（例1）ユーザーの登録機能

メールアドレス、パスワードが必須、名前、住所を任意で指定出来る。

また機能要件として、以下の条件を満たす必要がある。

- 既に登録済のメールアドレスは登録出来ない
- RFC 5322 & 5321に違反したメールアドレスは登録出来ない
- パスワードは最低でも12文字、半角英字の大文字、小文字、半角数字が含まれている必要がある

非機能要件として以下の条件を満たす必要がある。

- 100rpsのリクエストに耐えられる事
- 100rpsの負荷がかかった状態でリクエストの99%が150ms以内に収まる事

（例2）ユーザーの検索機能

ユーザーID、メールアドレス、名前での検索が出来る。

また機能要件として、以下の条件を満たす必要がある。

- ユーザーIDが指定された場合は完全一致検索を行う
- 入力したメールアドレスの中に `@` が含まれる場合は完全一致検索を行う
- 入力したメールアドレスの中に `@` が含まれない場合は前方一致検索を行う
- 名前が入力された場合は前方一致検索を行う

非機能要件として以下の条件を満たす必要がある。

- 200rpsのリクエストに耐えられる事
- 100rpsの負荷がかかった状態でリクエストの99%が100ms以内に収まる事
  • データの境界での分割
    • （例1）ユーザーの登録機能を元に説明
      • 最初はメールアドレスとパスワードだけ登録できる段階でPRを作る
      • そのあとで名前、住所を登録可能にするPRを作る
  • 横断的な関心事での分割
    • （例1）ユーザーの登録機能を元に説明
      • 最初はRFC 5322 & 5321に準拠するという条件は無視した状態でPRを作る
      • 「パスワードは最低でも12文字、半角英字の大文字、小文字、半角数字が含まれている」という条件はいったん無視してPRを作る
  • 操作の境界での分割
    • （例2）ユーザーの検索機能を元に説明
      • 最初はユーザーIDでのみ検索できる状態でPRを作る
  • パフォーマンス制約での分割
    • 200rpsのリクエストに耐えられる事などの非機能要件は無視した状態でPRを作る

こんな感じでPRを分割すれば大きなPRになることはあまりないです。

またこれらは、課題自体の分割にも利用できる考え方です。

複雑な問題を単純な小さな課題に分割すると、バグや認識の齟齬も起こりにくいので、これらは常に意識したいです。

PRや課題の分割手法について、もっと詳しく知りたい人は下記の書籍などを見ると良いでしょう。

（参考）ユーザーストーリーの分割
（参考）アジャイルな見積りと計画づくり（12章 ユーザーストーリーの分割が参考になる）

早めにDraft Pull Requestを出す

Draft Pull Request を早めに作成し、レビュアーに通知を送りましょう。

作業内容を早めに共有することで、認識の齟齬があった場合、早めに軌道修正できます。

Gitは通常、変更ファイルが1つもない場合コミットはできませんが、 git commit --allow-empty を利用すれば変更がなくてもコミットを行なうことが可能です。

git commit --allow-empty を使って作業開始と同時にDraft Pull Requestを出し、以降はローカルでコミットした内容を随時反映させるスタイルがオススメです。

ちなみにDraft Pull Request登場以前はPRタイトルの先頭に [WIP] をつけることでそのPRはまだ作業中という事を表すことが多かったです。

Draft Pull Requestは明示的にレビュー可能状態にしない限りは間違ってマージされる事がないので、こちらのほうが安全です。

DraftPR2

個別説明が必要な箇所はコメントをつける

説明を記載する際は対象箇所にインラインコメントとして補足説明を記載するとわかりやすくなる場合があります。

こういう場合はPRの説明欄に書くよりも、インラインコメントを記載しましょう。

PRComment

※「PRの説明欄に書くべき事」に書いてある必要な情報を記載してあるという前提です。

返信は同じスレッドに対して行なう

もらったレビューコメントに対して、返信を行なう際は必ず同じスレッドに対して返信を行ないましょう。

ThreadComment

たまに、PRの下部にある新規コメント欄から返信を行なうケースを見かけます。

ここに書かれたコメントはスレッド機能が使えないので、あとで流れが追いにくくなります。

コメントを行なうときはスレッド機能が使える、コード上のインラインコメントを使っていきましょう。

ThreadComment2

なお、インラインコメントは該当行を消してしまうと、Files changed から消えてしまうことがあります。

しかし、PRのトップページである Conversation には必ず残っているので、そこから返信するようにしましょう。

ThreadComment3

修正内容のコミットIDをリンクで伝える

レビューコメントに対して修正をした際はその修正内容がわかるように、該当のコミットIDをコメントに記載しましょう。

CommitID

CommitID1

コミットIDがあると、レビュアーはリンクをクリックするだけで、意図した修正が行なわれているか確認できます。

CommitID2

これによってコニュニケーションが円滑に進むようになります。

番外編（コミットメッセージを工夫する）

PRの話から少しズレますが、コミットメッセージをわかりやすく記載することも重要です。

変更内容に関しては、差分を見ればわかるので、なぜその変更をしたのかをコミットメッセージに記載しましょう。

以下のような方法を使ってコミットメッセージを統一するのも良いでしょう。

コミットメッセージに関しては記事の本題からズレるので詳しい説明は割愛させていただきます。

まとめ

今回のまとめです。

  • PRの説明欄には以下を記載する
    • GitHub issueのURL
    • このPRの対応範囲
    • 変更内容＆変更理由
    • レビュアーに重点的にチェックして欲しい点
  • PRは適切な大きさに分割する
  • 早めにDraft Pull Requestを出す
  • 個別説明が必要な箇所はコメントをつける
  • 返信は同じスレッドに対して行なう
  • 修正内容のコミットIDをリンクで伝える
  • 番外編（コミットメッセージを工夫する）

あとがき

新しい現場にJOINした際、PRの運用について質問される事が何度かあったので今回記事にしてみました。

自分は常に以下の事を意識してPRを作成しています。

  • あとで見た人のために情報を残す（未来の自分も含む）
  • レビュアーの負担を減らすために創意工夫する

PRは開発者のコミュニケーション手段ですが、書き方を工夫すれば良いドキュメントにもなります。

リモートワークがメインとなった今の時代においては、これらのような非同期コミュニケーションスキルの重要性が増しています。

この記事の内容を実践すれば、コミュニケーションコストや認識の齟齬を減らすことができます。

今回はレビューイ目線の記事でしたが、レビュアー目線の記事も書く予定です。

以上になります。最後まで読んでいただきありがとうございました。

明日はコーポレートIT担当の TaketoWakabayashi さんです。お楽しみに。

Discussion