エンジニアの id:cheezenaan です。先月11月の話ですが、かのラーメン二郎目黒店から徒歩圏内という恵まれた立地へと引っ越してきました。これまで片道1時間弱かかってきた通勤時間が大幅に短縮できて、クオリティー・オブ・ライフ(QOL)の向上に強く貢献したと実感しています。ちなみに好きなコールは「ヤサイニンニクアブラカラメ」です。
さてピクスタ開発部では GitHub の Pull Request 機能によるコードレビューを導入しています。新規機能の開発から既存機能の改修まで、自分以外のメンバーによるレビューを通過した内容だけが master ブランチへマージされ、本番環境へリリースされます。
みなさんの会社や個人での開発でも、開発プロセスにコードレビューを導入している方が多いかと思いますが、今回は「私が他のメンバーへコードレビューを依頼する際に、どのような点に気をつけてプルリクエストを作成しているか」について、軽く話ができればと思います。
このエントリは、PIXTA Advent Calendar 2017 5日目の記事です。4日目は tokinaga による「未経験でウェブ業界に転職する前に準備したことと、その結果」でした。
こんなプルリクエストはいやだ(※個人の感想です)😫
コードレビューをしたことある方は、このようなプルリクエストを見たことがあるかもしれません。
- diff が10,000行超え😇
No description provided🤔- コミットメッセージから修正の意図が読み取れない😇
diff が10,000行超え😇
「あれもこれも…」と修正を加えていったら10,000行を超えてしまったのでしょう。diff が増えれば増えるほど、何をチェックすれば良いのかレビュアーは判断しづらくなります。
No description provided🤔
よほど自明で単純な修正であったり、プロジェクトに関わるメンバーがみな対象分野にくわしい場合は、Description を仔細に書かなくても問題ないかもしれません。しかしチーム開発の現場ではこのような都合のいい修正ばかりではありませんし、レビュアーとレビュイーとで業務知識やスキルに差がある場合が多いかと思います。「レビュアーは必ずしもレビュイーほど対象分野にくわしいわけではない」と留意する必要があります。
コミットメッセージから修正の意図が読み取れない😇
「なにを・なぜ修正したのか」という意図が読み取りづらいコミットメッセージは、レビュアーがプルリクエストを理解するにあたっての支障となります。たとえば fix の一言しかないコミットメッセージをレビュアーが見て、どう反応するでしょうか。ある機能に関する何かしらの仕様修正なのか、単に Rubocop や ESLint などフォーマッターの指摘修正なのか、想像できるでしょうか。また、コミットメッセージと実際の修正内容が一致しないコミットも、レビュアーに混乱させかねないので避けたいところです。
やっていること⚒
ピクスタ開発部にジョインしてから1年半、レビュアー・レビュイーとして数々のコードレビューを経験してきたなかで、「ここに気をつけると、コードレビューはもっとうまくいくかも…」と思うポイントをご紹介します。
- 1つのプルリクエストで修正する diff はせいぜい200行まで✂️
- Pull Request テンプレート機能を利用して必要な情報を書き込む✍️
- コミットの粒度はストーリーを実現するためのタスク単位に揃える📏
1つのプルリクエストで修正する diff はせいぜい200行まで✂️
diff が増える要因として、プルリクエストの中に大量のストーリーが含まれているために修正規模が大きくなっていることが主に考えられます。ここでいう「ストーリー」とは 「○○(who)が○○できる(can what)」 という形式で表現できるものを指します。
まずはストーリーを細かい粒度に分割していきます。できれば各スプリント前のプランニングの段階でできているとベストです。実装を進めていく中で diff が数百行単位で増えてきた場合は途中でも作業を一時中断し、ほどよい粒度にブランチを分割してから作業を再開します。
個人的には、1つのプルリクエストの diff が200行以内に収まると、レビュアー・レビュイーともに負担を覚えることなくコードレビューを進められると感じています。大きくなったストーリーを分割するコツは『アジャイルな見積りと計画づくり』が非常に参考になりました。
アジャイルな見積りと計画づくり ~価値あるソフトウェアを育てる概念と技法~
- 作者: Mike Cohn,マイクコーン,安井力,角谷信太郎
- 出版社/メーカー: 毎日コミュニケーションズ
- 発売日: 2009/01/29
- メディア: 単行本（ソフトカバー）
- 購入: 74人 クリック: 764回
- この商品を含むブログ (226件) を見る
Pull Request テンプレート機能を利用してレビューに必要な情報を書き込む✍️
ピクスタでは他のメンバーへコードレビューを依頼する前に、GitHub が公式に提供する Pull Request テンプレート機能を利用した Description 記載ガイドラインに沿って、レビューに必要な項目を埋めるようにしています。
テンプレートでは主に以下の項目を記入するようになっています。
- 概要・技術的変更点: そもそものストーリーの説明や、ストーリーを実現するために行った変更点。文字だけで伝わりにくい場合は適宜スクリーンショットも添えます
- 見どころ: とくに見てほしいポイント、あるいは見なくてもよいポイントを書きます
- レビュー完了希望日: そのプルリクエストの緊急度を伝えます。不具合対応なので今すぐ見ないとマズそうだとか、軽微な改修なのである程度日を置いても大丈夫だ…と伝わります
- テスト項目: 自動テストやブラウザ上での目視・手動操作による確認項目を書きます
テンプレート機能のくわしい使い方は、GitHub 公式のリンクを参照ください。
Creating a pull request template for your repository - User Documentation
コミットの粒度はストーリーを実現するためのタスク単位に揃える📏
ひとつのコミットでは、ひとつの変更だけを扱うようにします。具体的には以下のことを意識するとうまくいく場合が多いです。
- ライブラリの導入や既存コードのリファクタリング、機能の追加・改修でおおまかに作業を分ける
- ESLint や Rubocop などチェックツールの指摘修正はそれとわかるように
Lint等のメッセージをつける
たとえば「既存のページに試聴プレイヤーを組み込む」場合で考えてみると、このように分割できるのではないでしょうか。
- ライブラリの導入(
yarn installやbundle installの結果だけをコミット) - ページへの組み込み
- 既存コードのリファクタリング
- 機能追加A
- 機能追加B
- Rubocop や ESLint などのフォーマッターによる指摘対応
- …など、実際にはもう少し細かく分割することもあります
PIXTA のサービス開発ではタスク管理に Backlog を使用しており、実際のコミットメッセージには [Backlog のチケット番号] (作業内容) という形式で記録しています。チケット番号と作業内容をコミットメッセージで紐づけることで、コードの変更意図を追いかける際に「コードに対して git blame する → 該当するチケットを Backlog から探す → 施策の背景やあらましをつかむ」という一連の動作がスムーズにできます。
適切な粒度に作業を分割できていれば、コミットメッセージもおのずと修正内容に沿ったシンプルな内容に決まるため、読み手にも意図が伝わりやすくなります。それでもなお「何をメッセージに残すか？」と悩んだときは、偉大なるプログラマである @t-wada 氏のツイートを思い出すといいかもしれません。
コードには How
— Takuto Wada (@t_wada) 2017年9月5日
テストコードには What
コミットログには Why
コードコメントには Why not
を書こうという話をした
まとめ
「私がコードレビューでプルリクエストを作成する際に気をつけていること」をつらつらと挙げてみましたが、これらの根底にあるのは「チーム開発をともに進める他のメンバーへの細かい気配り・思いやり」なのかなと思いました。
コードレビューはレビュアー・レビュイーがともにコードと向き合って、よりよいプロダクトをつくるための習慣です。小さな工夫や心づかいを積み上げて、このすばらしい習慣をよりよく・長く続けていきたいものです。
そしてまた、次のアドベントカレンダーがはじまるのです
6日目の記事は、私と同じ2016年8月入社の kenue が担当します。ご期待ください！
エンジニア募集中！
ピクスタでは、細かい気配りをしながらチーム開発を進めるのが得意なエンジニアを募集しています。
