コードレビュー、これまでいろんなプロジェクトで経験して、意外と使われていないノウハウがあったり、風習が違ってつらみがあったりしたので、いろいろまとめてみる。

指摘事項について

よくある話
- 駄目コードを憎んで人を憎まず。駄目なのはコードであって人格じゃない
- 指摘する人は人格攻撃せずにコードのどこが悪いのかを指摘しましょう
- 指摘される人も、言われているのはコードの問題であって人間の問題じゃないので、素直な心で受け止めよう

この辺はみんな知ってると思うので略。ぼくが思う大事なルール

コードレビューで指摘された内容は、対応必須ではない

理由： 対応必須にすると、「これ言ったらリリースできなくなるよね」みたいな忖度が発生してコメントできない人が出現するから。

絶対ダメとは言わないけど、あまりよくはない、みたいな指摘については、そのときは急ぐからリリースするけど、次回から気をつけるとかがありうるよね。そういう指摘ができなくなる対応必須ルールはみんなの成長を阻害するのでやめたほうが良い。

• Q：なんでも指摘を無視してリリースできちゃう？
• A：そういうことにはならない。"Approveについて"参照

Approveについて

レビューアーは、「このPRがプロダクトにマージされて良い」と思えばapprove(👍)をつける。これが一定数集まったらリリースしてOK、というのが一般的なルール。

レビュアーの行動

approveだけする

問題ない。でもできれば、ちょっと感想とか、「こういう観点で見ました」「こういう点が気になったのでチェックして、問題ないことが確認できました」みたいなコメントがあると、あとからレビューする人は別の観点からチェックすることができて嬉しい

approve＋コメント

単なる感想とか、気になるけどまあ直さずにリリースしてもいいでしょ、程度の問題のとき

approveせずにコメント

この指摘を直さないのなら自分はリリースに合意できない、という意思表示。
ただし、approve数のルールによっては、自分が合意しないけどリリースされることはありうる。絶対ダメなコードについては強く指摘して他の人の注意をひこう

5/29 追記 Twitterで指摘いただいた。絶対ダメなコードはこれで止めるのが良いね。

GitHubのプルリクには "Request Changes" というものがあるので、リリースに合意できないときはコメントだけじゃなくて "Request Changes" を使うと良い。 https://t.co/NDWUEIyVGL

— 神速 (@sinsoku_listy) 2019年5月27日

Approveいくつ問題

そんなルールのもとで、いくつapproveを集めたらリリースしていいことにするか

一人合意すればOK

一定数までは、リリースに必要な合意数とコードの品質は比例するので、一人だと品質が微妙になる。
でも、スタート直後のベンチャーなんかだと、スピードを優先して一人合意にするのはアリ。あと、CTO的な人がいて、その人が合意したらOKというパターンもある。
１０人もいるのに一人合意でOKにすると、いい加減なOKを出すやつが出て致命的な問題が見逃されるリスクが出てくるので、もうちょっとしっかりやりたい

全員合意

あまりおすすめしない。少数ならやっても良いかも知れないけど、１０人のチームでこれをやると、それぞれ９人分のPRを見ないといけなくなって死ぬ

２－３

これくらいがおすすめ。このやり方のキモは、合意を集めるためにある程度の指摘に対応しないといけないこと。

「指摘事項について」で書いたとおり、指摘事項は無視ができる。そのかわりレビューアーは、「直さないのだったらapproveしません」権を持つ。
レビューしてもらう人は、approveを集めるために、ある程度の指摘事項には対応しないといけなくなる。結果的に、一人厳しすぎる人の指摘はスルーできるけど、みんなが駄目だと思うような項目はスルーできなくなる

マージボタンを押す人問題

合意を集めた後、誰がマージボタンを押すか問題。
誰でも良さそうな気がするけど、いろいろ妙味があるので、これも決めておいたほうが良い

特定の人が押す

チームのリーダー的な立ち位置の人がいて、その人がマージボタンを押す。
当然その人は全部のコードに目を通すことになるので、プロダクトについてほぼ何でも知っている人みたいな位置になる。

そういう人がいるとみんな頼もしいし便利なんだけど、その人の負荷はとても高く、退職したり爆発四散してしまったりするとプロジェクトが一気にピンチに陥るので、初期のベンチャー以外ではあまりおすすめしない。

別案として、プロダクトオーナーみたいな人が押すという手もある。技術的に深く理解している必要はなくて、「エンジニアがみんな合意してるから大丈夫だろ」みたいな感じで押す。どんな機能が導入されたかプロダクトオーナーが把握できるのがメリット

レビュイーが押す

レビュイー、つまりPRを作ってレビューしてもらう人がマージボタンを押す。とてもおすすめのルール。

このルールのポイントは「👍が集まったら、マージボタンを押しても良い」にしておくこと。つまり、「👍集まったけど、この機能にとても詳しいAさんにも見てほしいからもうちょっと待とう」「👍集まったけど、この指摘は重要なので対応しておきたい」というようなことが出来る。
逆に「いまはスケジュール的に押しているから、最低限の👍でリリースする」という判断もありうる

ひよっこ問題

駆け出しエンジニアみたいな人がいて、普通のコードレビューが成立しなくなる問題に対する対処

甘すぎるapprove問題

👍一個でリリース可能で、ひよっこが合意したらそれでリリースできてしまう、みたいな問題。

放置してしまうと「責任が取れないからapproveしない」みたいな遠慮をすることになってしまって、成長のためにとても望ましくない状態になる。ひよっこが👍したあとで先輩から厳しい指摘がついて「そういうところを見るのか！」ってなる方が良いですよね。

必要な👍数を増やすという手もあるんだけど、「マージボタンはレビュイーが押す」ルールの導入がおすすめ。これがあれば、「彼の合意だけでリリースするのはまずいのでもうひとり見てくれるまで待とう」というブレーキをかけることが出来る。
逆に、「些細なPRなので彼一人でも良いだろ」という判断もできて、柔軟な対処が可能になるのもうれしい。

レビューが成立しない問題

ひよっこの書いたコードがアレすぎて、
- どこから指摘していいかわからない
- 指摘が大量について対処できなくなる
- ひよっこの心が折れちゃう

等に関する対応。

「PRを出す前に、隣の人にチェックしてもらってください」がオススメ。メンターさんとか教育係でもいいし、持ち回りでもよい。

巨大PR問題

リファクタリングとかで、何十ファイルにも及ぶ巨大PRができてしまって誰もレビューできなくなる問題

分割する

approveをあつめるのはレビューイーの責務なので、見てもらえる程度のサイズに分割して小さなPRに分ける。

理想的なんだけど、大変な修正をしたのにPR分割しろと言われると、心が折れちゃうよね...

説明会

時間を決めて会議室にあつめたり、オンライン会議をして、コードについてみんなに説明する場を持つ。その場で質疑応答して、最後にみんなの合意をもらうことで、approveの代わりにする。

まとめ

過去いくつかのプロジェクトでコードレビューして、これは良かった、これは辛かった、というノウハウを書いてみました。いかがでしたか これが絶対のルールというわけじゃないので、うちではこうしているとかあれば聞かせてください。