どうも、こんにちは。
ヒコーキ好きのみこやんです。
新型コロナの自粛要請も解けてようやく…となったものの、それとは別に最近なかなかヒコーキに乗れていなくて、しょんぼりな日々が続いております。
それはともかく…。
前回は「コードレビュー規約を作る」という制定編をお届けいたしました。
多くのことをルール化してしまうと、結果的に誰にも読まれず守られない規約が誕生してしまいかねないことから、いかにエッセンスだけを簡潔に文書化するかについて考えてみました。
今回は、コードレビュー規約の運用面のルールの作成について見ていきます。
運用されるための手順づくり
現状のコードレビュー手順を確認する
弊社はGitHubを用いたプルリクエストによるコードレビュー運用をしております。
おそらく、ほとんどの開発チームがこのスタイルを取り入れていると思います。もはやGitHubとプルリクエストはアプリケーション開発におけるデファクトスタンダードと言ってもよいでしょう。
コードレビューの運用についてルール化する場合、「ガラッと変更する」といった例外がない以上は、なるべく現存のルールを取り入れた上で制定していく必要があります。
当然ながら、従来のコードレビュー手順からかけ離れてしまうと、チームメンバーの多くが混乱してしまいますので、ルール化する上で注意する必要があるポイントの1つです。
まずは現状の手順を、簡単な箇条書きで書き出して見るのがよいでしょう。
弊社のLetro開発チームの場合は、おおまかに以下のような流れでした(運用手順をルール化しても、基本の流れは変えていません)。
- レビューイは、プルリクエストを出す前に静的解析ツールでコードがエラーにならないことを確認する。
- レビューイは、プルリクエストを出した直後にSlackのレビュー依頼チャンネルに投稿する。
- レビューアは、依頼されたコードレビューを開始する。
- レビューアは、コードレビューが完了したらGitHubのプルリクエストをapproveする。
- テックリード(もしくはそれに類するポジションのエンジニア)がプルリクエストをマージする
以上がおおまかな流れです。
細かな作業やコミュニケーションは、この流れの中では省いてています。
また、ルール化以前は人によって一部異なる運用をしたり、例外が散見することもありました。これらはルール化にあたって阻害要因となるので、現状を書き出す際にあらかじめ取り除いておきます。
守るべき手順を書き出す
「ルール化」とは、開発チームのメンバー全員の決まりごとです。
さきほど洗い出した共通の流れの中に、全員が例外なく守るべき要素を織り込んでいくことになります。
織り込むべき要素や考慮点は、以下が基本となります。
- コードレビューの手順を、ミスなくスムーズに完了させるための要素
- 不必要な作業や工程に費やすリソースをなくすための要素
- 開発チームのメンバー全員が、短期間で理解できるようにするためのシンプルさ
「制定編」からの繰り返しになってしまいますが、運用チームのメンバーの誰もがすぐに理解でき、実践できるぐらいのシンプルさがなければ、「規約作って運用されず」となってしまいかねません。
最後の項目は、そのための考慮点となります。
方針が見えてきましたので、あとは織り込むべき要素の具体的な内容を決めていきます。
たとえばLetro開発チームでは、コードレビュー時には適当にレビューアをアサインしたり、実際にはCIなどで静的解析を回していないなどのレビューイもいました。
これらをルールの中に織り込んでいくことで、従来のコードレビューの流れを大きく崩すことなく、一定の堅牢性を担保したコードレビューの運用手順を提供することができるようになります。
ちょっと書き出してみましょう。
- プルリクエストには必ず規定のフォーマットのDescriptionを記載する。
- メインとなるレビューアを必ず1名指名する。
- レビューイは、レビューアがコードレビューしやすくするために、個々のレビューサイズを小さくする。
- プルリクエスト時にコンフリクトが発生したら、そのプルリクエストのレビューイが解消する。
- プルリクエストは、メインのレビューアが状況を判断した上で、開発ブランチにマージする。
大まかすぎず、細かすぎない、ある程度の粒度の「守るべき」要素に絞り込み箇条書きにしました。
たとえば、これまでのルールではSlack経由でレビューイがコードレビューを依頼するだけで、特定の誰かを指名する前に、暗黙的にレビューアが決まっていました。
しかし上記の書き出しで「メインとなるレビューアを1名立てる」ことが明示されるため、曖昧なレビュー依頼ではなく最低でも1名のレビューアがコードレビューに取り掛かる状態を生み出せます。
このように、手順の中で不可欠な要素を確実に押さえるポイントを「守るべき要素」とすることで、コードレビューの運用の堅牢性が保たれるようになります。
実運用に向けた取り組み
実際の運用に即した形で深掘りしドラフトを作成する
洗い出した箇条書きをベースに、実際の運用シーンを想像しながら、理解しやすい文章になるよう肉付けを行なっていきます。
たとえば「プルリクエストには必ず規定のフォーマットのDescriptionを記載する」といった別の様式などが必要になる箇所では、ちゃんとその様式を整えた上で、規約としての形にもっていきます。
実際に、Letro開発チーム向けに作ったものを、以下に紹介します。
- 開発者は、自身の開発用featureブランチを、マージ対象ブランチに対してPull Request（PR）を提出する。その際、以下を遵守する。
- Descriptionに詳細を明記する。
## タスク
チケットに記載されている見出しを記載## 関連チケット
– LETRO_1234## レビューポイント
– レビューして欲しいポイントを記述## 動作確認
– どの環境でどんな動作チェックをしたか
– 動作確認をした事についてスクショなどがあるとわかりやすくて良い
– NotionなどでまとめてるならそのURLを貼るだけでも良い
– Verifyでしかテストできないなどの留意事項があればその旨わかるようにする - すべての開発者・テックリード・アーキテクトからメインレビューアを1名指名（assign）する。
- メインレビューアは、必要に応じてサブレビューアを1名以上指名することができる。
- Descriptionに詳細を明記する。
- レビューイは、Pull Requestを提出する前に、以下のことを確認する。
- レビューイは、レビューアがレビューしやすい粒度（Pull Requestの大きさ）を開発前に合意形成し、その粒度でのPull Requestを可能な限り守ること。
一般的には、400行程度以上のレビューサイズになると、問題発見率が下がると言われているため、400行未満となるようにすること。 - CIもしくはローカルでの静的解析実行でエラーになった場合、通過（pass）するように修正してからレビュー依頼する。
- Pull Request時にコンフリクトが発生した場合は、レビューイが解消する。
- レビューイは、レビューアがレビューしやすい粒度（Pull Requestの大きさ）を開発前に合意形成し、その粒度でのPull Requestを可能な限り守ること。
- レビューイはSlackのレビュー依頼チャンネルに対して、PRに関する概要とともにレビュー依頼を投稿する。
- レビューアは、レビュー方針に基づきレビューを実施する。
- レビューアは、レビューが完了したら、上記2.の投稿に対して承認（approve）する。
- 「LGTM」など、わかりやすいアイコンをチーム内で決めて合意したものを承認マークとする。
- レビューイは、全てのレビューアからapproveがついた段階でマージが可能となるため、自身の開発ブランチのマージを、メインレビューアに依頼する。
- メインのレビューアがタイミングをみて承認されたPRをマージする。
さきほどのDescriptionについては、その様式・書式のテンプレートを掲示することで明確化しています。
実際には、GitHub上でテンプレートを設定することで、プルリクエストを作成した際に自動的にテンプレートが埋められていて、中身を書き換えるだけになるようにするといった工夫が必要となるでしょう。
レビューイがコードレビュー依頼を出す前までに果たすべき責務についても、ある程度明示されています。
たとえば、レビューしやすいサイズでのcommitを、事前にレビューアと合意形成することで、レビュー時に巨大な差分にレビューアが辟易するような状態を可能な限り回避できる手段を講じておくなどが、その例となります。
実際の運用で使用するルールであるため、「400行程度」という具体的な数字が記載されています。
キャラクターベースのコミュニケーションで失われがちの「雰囲気」を伝えるために「LGTMなどのアイコンを使用する」といったことも、具体的に明記しています。
これらはツールによっては使えない場合もあるため、実運用に即したもので検討するほうがよいでしょう。
フィードバック改善を経て、全体の合意をとる
ひとまず形になったルールは、まだドラフトの状態にあります。
これを運用する前に、適用する開発チームや組織にドラフトを公開し、フィードバックを受ける必要があります。
最低でも、1回は全体ミーティングや説明会などを開き、ドラフトを示した上で意見交換することが不可欠です。
自分が良いと思っていても、やはり見落としや誤認などはどこかにあります。
また、従来の問題点を改善できると思って策定したドラフトであっても、第三者からの目線では、それがまったく改善になっていないか、逆に現場的には改悪にさえなっている可能性も否定できません。
さまざまな目を通した上で、ドラフトをさらに洗練させていく作業を、怠ってはいけません。
もちろん、私がLetro開発チーム向けのコードレビュー規約を策定した際にも、こうしたフィードバックミーティングを設けました。
前回もお話した通り、特にLetro開発チームはベトナムにも開発拠点があり、それぞれに卓越した開発メンバーが揃っています。
彼らにもルールを適用する異常、彼らからのフィードバックも貴重な意見となり得ますから、十分に時間を使って説明する必要がありました。
フィードバックをもとにドラフトを修正し、それをまたフィードバックミーティングなどに諮る…といった改善イテレーションを回していきます。
そして全体の合意が取れたとき、はじめてリリースの日の目を見ることになります。
運用しながらフィードバックをもらい調整する
一度ルールを決定し、運用に乗せたら、それで完了というわけではありません。
実際に運用してみて、本来想定していたこととは異なることもあったりするでしょう。
こればかりは、どんなにフィードバックをもらって合意をとったとしても、全体が想定していないことが発生すれば、その問題を解消しなければなりません。
幸いなことに、Letro開発チームでは現時点ではまだ大きな変更が発生してはいませんが、もし発生したら、問題を打開する改善をルールに盛り込んだ後に、先に行ったようなイテレーションを実施して「ルール改訂」することになります。
まとめ
コードレビュー規約に関するお話を、「制定編」「運用編」と、2回に分けてさせていただきました。
こうしたルールづくりは非常に大変ではありますが、その存在の有ると無しでは、均一的なレビュー品質の担保という面で、大きな違いが見えてくると思います。
コードレビュー規約が大きく機能する場面は、主に以下の3つあると思います。
- 新卒・中途などの社員が入社した場合
- 実際の開発シーンにおいてトラブルやすれ違いなどを解消する場合
- 開発チームにおける心理的安全性を担保したい場合
1のように、新しいエンジニアを開発チームに迎える場合、コードレビューについて感覚的な解釈がなくなるため、容易に理解・履行に至ることでしょう。
2のような場面は想像したくはありませんが、コードレビューにおいて何らかのコミュニケーションの衝突が生まれてしまった場合、チーム全体がルールに基づいた判断をすることが可能です。
そして、2のようなことが発生しないようにするための確約事項としての、3の意図も案に含まれていると私は思っています。
コードレビュー規約があることで、レビューアもレビューイも自分が思っていること、コードについて信じていることを、安心して伝え合うことができる「心理的安全性」を確保するには、こうしたルールは一定の役割を担えるのではないかと、私は信じています。
前回、今回と、コードレビュー規約に関するお話をご紹介いたしました。
本記事を読んでいただいた皆様のお役に立てれば幸いです。
最後までお読みいただき、ありがとうございました。
プログラミングもサーバもフロント(チョトだけ)もインフラ(オンプレ中心)もwebもゲームも組み込みも幅広く何でも雑多にやってきた古株ですが、フルスタックではありません。好きなものはプログラミングと計算機科学。我々と一緒に好きなプログラミングを楽しみませんか？
