レールに乗るとレールに乗らない？
昨今身の回りだと、すっかり Github のプルリクベースの開発で、「プルリク出して Github 上でみんなでレビュー」そんなやり方が当たり前になってきました。
レビュー制度自体が崩壊しがちな開発現場で、このレールに乗ったやり方で、どこの現場も安定してレビューライフを導入できているように思います。
ただ、どんな話でも共通ですが、レールに乗ると、レールに乗らないものを忘れがちだと。
盛大な手戻りストーリー
レビューイー
「さあ、プルリクーしたーぞー」
レビューワー１
「それ全然作り方ダメじゃない？」
レビューワー２
「その実装だとパフォーマンス悪いね」
レビューイー
「めっちゃ手戻りー（＞＜」
程度の差はいろいろあれど、なんかこういうケースをよく見かけるなぁと。ある程度は仕方ないけど、あまりに大きい手戻りがなんども連続して続くと、プロジェクト的にはつらいでしょう。
盛大なアンチパターンストーリー
それを心配した逆のアンチパターンに
発展する可能性もあって...
レビューワー
(指摘すると手戻りになるし嫌われそうだから)
「手戻りが大きいからこれ指摘するやめよう」
レビューイー
(これを了承すると手戻りが発生してつらくなるので)
「いや、その修正の必要はないんじゃないですか！
そもそもその指摘自体がおかしいと思います」
マネージャー
(手戻りが発生するとプロジェクト進まないから)
「みんなー、レビューは "ほどほど" になー」
プルリクだけがレビューなの？
jflute的には...
豪華絢爛なコードを積み上げた後に、初めてのレビューってのがおかしくない？
「プルリク方式でレビューをしよう！」それはそれですごくOKなことですが...
「プルリク方式だけでレビューをして、他のレビューは禁止しよう！」
いやいや、そういうこと言われたわけじゃない。
たまたまアドリブでやっていたレビューの一部が、プルリクという形で定型化されただけであって...
「もの作りをする上での根本的な確認作業」が、なくなるわけじゃない。
なくしていいわけじゃない。
早く良いものを作る判断は自分で
他人にレビューをしてもらうにしても、そのプログラムをいかに早く良いものに仕上げるかは、そのプログラムを作ることを任されたディベロッパーの責任です。
レビューで指摘されて手戻りが発生したら、それはそのディベロッパーの責任です。
もちろんレビューの指摘が間違っていなければね。いや！仮に間違ってたとしても、それに対してしっかり納得感のある説明をするのは、ディベロッパーの責任でもあります。
「いやそんなつらい、手戻りでめっちゃ残業が...」
「最初から求められてるコードをぴったり出すって無理...」
Yes, な・の・で、別に豪華絢爛なコードを積み上げてから、チェックしてもらうじゃなくてもいいんじゃない？
ということで、いろいろあるはず。
1. 実装する前レビュー
2. 実装してる最中レビュー
3. 実装し終わった直後自己レビュー
4. 実装し終わった後プルリクレビュー
1. 実装する前レビュー
「えっ、実装してないのレビューできんの？」
できます。
どんな実装をしようとしているのか、先輩やリーダーや周りの人にイメージを伝えること。
(ホワイトボードやノートでちょこっと図を描いて)
レビューイー「こんな風に実装しようと思ってます」
レビューワー「ああ、いいんじゃない」
レビューワー「あっ、そこはこうかな」
これだけで、いきなり道を踏み外した実装に突き進んで、プルリク時にぜんぜんちがうよーってのを防げるかもしれません。
...
jfluteは、こういうレビューを定形運用してたことがあります。
コミュニケーション図を "ざっくり" 描いてもらって、jfluteがレビュー、おおよそのクラス構造や使うライブラリの方向性などが変な方向に行ってないことだけをチェック。そこで議論が生まれて早い段階で解決することも多いので、とても良かったです。ディベロッパーも安心感あったようです。(これはSIの現場でした)
コミュニケーション図は概念図レベルの粒度なので、細かい変更があってもそんなに影響は受けないので、そのまま事後のドキュメントとしても活用。
プルリク時に、「そこはLogicでしょ、そこはServiceでしょ」って大まかなクラス構造を指摘されるのはちょと遅すぎる。
2. 実装してる最中レビュー
「えっ、まだ動かないのにレビューできんの？」
できます。
コードの体裁や、クラス構造、実装のスタイル、パフォーマンス懸念など、すでにその段階で指摘できることはいくらでもあります。
なんか実装してて不安に思ったら、早い段階で見てもらったほうが全員Happyです。レビューワーは、あとで巨大な「うわぁ」を見せられるより、先に指摘できる方が全然嬉しいわけで、面倒とは思いません。
...
jfluteは、こういうレビューを定形運用してたことがあります。全員ではないけれど、自らコードに不安に思った人、もしくは、まだコーディングが未熟で不安な人、もしくは、まだ入りたてで現場のポリシーを習熟してない人に対して、「テストとかまだ、あんまり動いてない、保留もいっぱい」という段階でいいので出してもらって、「コーディング基礎レビュー」という名の特別レビューを。(これは事業会社でのサービス開発の現場でした、わりと最近^^)
プルリク時に、しょうもないレビュー指摘が減るように。プルリク時は、もっと業務的な重要ポイントや、致命的なパフォーマンス劣化、レアケースのバグなど、そういった高レベルのレビューに集中してもらいたいなって。
【追記】ここ独立したエントリ書きました:
実装してる最中レビュー、のタイミングって？
3. 実装し終わった直後自己レビュー
「えっ、自分でレビューできんの？」
当たり前だ！(^^
レビューの制度はとても良いのですが、いつもジレンマに思うのが...
人間、チェックしてくれる人がいると、自分でチェックしなくなる
こういう本能を持っていると思います。
これは、DBFluteハンズオンのレビューしてると、この辺すごくよくわかります。jfluteをコンパイラーだと思って出してくるとかw。まあ、プログラマーだけの話じゃないですよね、これは。人類の問題です。
でも、人は知恵と意志で問題を軽減することができるはずです。jfluteはよく「指差し確認をしろ」と言います。比喩的な表現ですから、実際に指差す必要はないですが、5分でいいからプルリク前に自分のコード眺めてみなと。「書いてる最中にたくさん見てるから...」と思っても、
手を止めて眺めるとガラリと見える世界が変わるよ
...
実際、(極端じゃなければ)評価が下がるわけじゃないかもですが...
プルリクレビュー時にあまり指摘が多い、手戻りが多いと評価が下がる
ってくらいの心持ちを持っていてちょうどいいくらい。実際、レビューの指摘が多いということは、「レビューコストが高いプログラマー」ということなので、その人の単価以上のお金がかかっているわけです。
ってこと考えたら、不安で不安で、自己レビューせずにはいらないはずです。その不安が品質の源なのです。
4. 実装し終わった後プルリクレビュー
そして、プルリクレビュー！
プルリクに出しているということは、「最終チェックをお願いしている」そんな感覚です。
なぜなら、OKならそのままマージするわけですから。来週にもリリースされるかもしれないわけですから。
なのでプルリクは、業務的・技術的な重要ポイントに集中したいものです。
実際にレビューしてると、根本的な指摘事項がたくさんあり過ぎると、重要ポイントが探しづらいものです。ゆえに、レビューノイズがたくさんあると、重要ポイントが見逃される可能性があるのです。
レビューはけっこう疲れるもの。専門のレビューワーがいるわけでもなく、みんな自分の仕事の傍ら合間にレビューしているので、一度にできるレビューには時間的・集中力的な限界があると思っていたほうが良いでしょう。
