📋

【コードレビュー】いくつかの施策を体験した結果、あくまでレビュー自体は1人が行うようにし、チームを少数に分けた方が良さげ

に公開

はじめに

対象

  • プルリクエストの運用に悩むITエンジニア向け。

プルリクの良さげな運用設計

プルリクエストはシゴデキ開発者に負荷をかけることが多く、メンバーの人数が多いと渋滞することもしばしば。故に様々な現場で、様々な開発者が、試行錯誤してきました。

プルリクの目的を明確にする

基本的には検収目的に留めるのが良いです。重箱の隅をつついていけばキリがありません。あくまでも納品物として要件を満たすかどうかに絞る。リファクタリングは別のチケットを作成して、案件とは別で対応する。一緒くたにあれもこれもと付け足すのはやめましょう。

教育やソースコードの理解など、様々な目的を相乗りさせることはできるのですが、それをメインにしてしまうと混乱します。あくまでサブの目的としておくと、忙しい時の優先順位や指摘内容の取捨選択で迷わなくなります。

最低限のチェックは全て自動化します。静的コード解析・コーディングスタイル・自動テストなど、人間がわざわざ確認しなくて済むように。個人の趣味嗜好でコードを修正させないこと。もし気に食わないのなら、チームで話し合ってルールそのものを変更しましょう。

すると人間が確認すべきは、要件を満たしているかやコードの保守性、セキュリティ的に問題がないかなどに絞られます。なるべく人間が行う作業を減らすのがコツです。こうすることで次項でより多くのメンバーを同チームに加えることができます。

必ず代表者を決める

最も上手く回っていたのは、あくまでレビュー自体は1人の代表者が中心となって行うようにし、1人で消化できる人数にチームを分割するというものでした。反りが合わない者同士は別々のチームに所属するようにします。

他のチームの方針に口を出すのはご法度です。代表者同士の意見がぶつかった場合は、対象のプルリクとは別枠とし、話し合いと必要が生じれば改修を行います。同時に解決しようとすると、相手チームにカチコミに行く感じになってしまったので、避けるようになりました。

勉強のために他のメンバーが同時にレビューするのは構いません。ただしそのコメントの指摘を採用するかも代表者が決め、最終的には必ず代表者が責任を持って承認するという形です。同じ立場の者同士を、同じプルリクに放り込まないようにします。

代表者はどちらかというと技術力ではなく、折衝能力を重視して決めます(とはいえ最低限の技術力は必要ですが)。ファシリテーターを経験しているか、この人ならできそうだなと思える方が望ましいです。

プルリクエストは技術力だけ・コードだけ見ればいいというものではありません。スケジュールや各メンバーの作業状況なども加味して、今どの状態になっていればいいかで判断を下すこともあります。そのとき技術だけに囚われると、かえって足かせになります。

代表者は調査・設計などの作業を行ってもいいですし、代表者がプルリクを出す作業を行う場合は、副代表を決めた上でレビュー依頼を出してました。暗黙的に副代表が決まっていたプロジェクトもありましたね。

その他運用型のメリット・デメリット

その他の運用方法をいくつか並べてみます。あくまで複数の現場で確認できたもののみです。あらかじめご了承ください。

おみくじ指名型

Slackでおみくじを作ったことがある方も多いのではないでしょうか。あの機能を利用してランダムに指名させることでレビュワーを決める方法です。

全員がまんべんなくレビューできる点で公平性があります。また過去に開発に関わったことがなく、仕様をよく理解していないコードのレビューをすることで、製品への理解度を深めることができます。またレビューの負担はメンバー全員に分散します。

しかしレビューの質は個々人のスキルに左右されることが多く、データベースに強い開発者がフロントエンドをレビューする、といったアンマッチが起きがちです。また自分の開発業務もあるため、仕様のキャッチアップにあてる時間がなくて結局だいたいでレビューOKにすることも。

全員が全員、万能型でスキルも揃っていれば成立しますが、現実的な話難しいでしょう。できている現場はよほど人事戦略がしっかりしてるのだと思います。

定量評価型

プルリクエストのノルマを「1日3コメント」「1プルリク1コメント」などと決め、ノルマを達成できない場合は公開処刑されるという方法です。そんなことがと思うかもしれませんが、複数現場で確認したので間違いないでしょう。営業ではないです、開発でです。

正直、この方法にはあまりメリットがありません。強制的にプルリクを見るには見るでしょうが、「子どもを見ておいて」で本当に見てるだけなのと同じ状態です。トリプルチェックやペンタチェックになると、もはや誰も責任なんて持ってません。社会的手抜きというやつです。

数字を達成するために品質の低いコメントが多くなります。また簡単なレビューに人が流れ、重いレビューが放置されることもあります。同じ個数なら簡単な方が楽ですから。また明らかに何分かかかる内容で、秒でいいねを付ける行為なども横行しました。

別記事で解説しようと考えていますが、この場合は定性評価も組み合わせて運用した方が良いと思われます。ただしできる開発者は上澄みの上澄みだとは思いますが。

複数レビュワー型

メンバー複数人でレビューを行う方法です。GitHubのApproveの最低人数を2人以上に設定し、規定の数の承認がもらえるまで通しません。

見るところを分担し、実質1人とすれば上手くいきます。例えば仕様面ではAさんが、実装面ではBさんが見るという具合です。もしくは意見が競合した場合は、話し合いの場を設ける、Aさんの意見を優先するなど、あらかじめ対処方法を決めておくのが良いです。

この方法で最も困るのが、レビュワーの意見が割れてプルリクを出した開発者がダブルスタンダードやダブルバインドの状態に置かれることです。AさんとBさんが各々自分の主張を丸投げした結果、両方の意見がぶつかってしまうという。

プルリクができるレベルのITエンジニア同士だと、価値観や設計思想がよほど一致する場合を除き、理想とするコードは微妙に違うことが多いです。なので、その衝突の度に時間がかかると3人全員の作業が止まる上、ストレスもかかるという不要なギスギスが生まれます。

セルフレビュー型

自分でレビューして、自分でマージする。完全自己完結型。開発者が自分1人だけだったときと、手練れ2人だけで時間がギリギリだったときは、これでした。CIが通ればOK、自分が見逃せば不具合になるか、検証環境でバグとして報告されるか。

メリットは何と言っても早いこと、これに尽きます。誰かのプルリクを見に行くことがないので、ローカル環境も脳味噌も切り替えなくて済みます。互いの作業が分かっていれば、競合することもありません。ある意味、とても作業しやすかったですね。

問題があるとすれば、タッグやトリオを組めるほどの相手になかなか巡り会えないこと。そして個人のミスがそのまま不具合になることです。自分の書いたコードを自分でチェックするので、怪しいところに気付けない可能性があります。

少数精鋭型の究極系みたいなとこがあるので、玉石混交のチーム開発ではやはり厳しいですね。

おわりに

プルリクエストのレビューほど、ストレス負荷が高くて、人間同士の距離が近い作業もなかなかないと思います。ぶっちゃけ一緒に働く人次第ではあるのですが、普段は大丈夫でもレビューになった途端に火花散ることは稀によくあります。

代表者を決めるってことは王様を決めるってことなので。独裁政治にならないようにだけは注意したいところです。ファシリテーターがいい理由はこれで、他のメンバーのことを気遣える方が望ましいです。自分ファーストな人に権力持たせちゃ駄目です。

状況によってはその他の運用型の方が最適な可能性があるので、メンバーの技術力やファシリテーター力、プロジェクトの状況を見て相談することになるのかなと考えています。

他にもあった筈なので、思い出したら運用型に追加します。

Discussion