レビューイは自分の成果物をどうレビューしてもらいたいか考えて伝えよう

「言われていたものを作りました。はいレビューしてください」というレビュー依頼の出し方をしていないだろうか？それはレビューイにとっても、レビューアにとっても、多くの無駄が生じるので、止めよう。

色々と思うことがあり、考えがまとまらなかったので、箇条書きで書き出すだけにする。

何が悪いのか？

• レビューアがレビューしていて、成果物のある部分について「どうしてそのように書いたのか」と疑問に思った時、理由や経緯が分からないので、レビューアが調べなくてはいけない。
• レビューアが調べて「多分こういう理由でこう書いたんだろうな」と推測した内容は、レビューイがそのように書いた経緯とは異なるかもしれない。そうすると、レビューイの意図が正しいかどうかを検証できておらず、思わぬところに悪影響を与えていることに気が付かないかもしれない。
• レビューアが調べ直しても分からなかった場合、レビューイに「何でこういう風に書いたの？」と質問する時間が発生する。レビューイは大概、レビュー依頼に出して「終わった」と思ったモノについては記憶が抜けているので、自分でもどうしてそうしたのか体系的に説明できず、時間がかかる。
• レビューアが、レビュー対象物に関する理由や経緯を明らかにするところに時間をとられ、本質的にレビューしたいところ (ロジックや判定条件の細かな部分など) に集中できず、バグを見逃してしまう恐れが高まる。
• 見逃されたバグは後工程になって見つかり、修正と再検証に莫大な時間をとられる。本番障害になってしまうと金銭的な損失にも繋がりかねない。

総合すると、レビューアに余計な労力がかかり、本質的なレビューができない。それによって後工程でバグを引き起こすなど問題が大きくなりやすいから、レビュー依頼の丸投げは悪なのだ。

なぜレビューイは理由や経緯を伝えずレビュー依頼を丸投げするか？

• レビューアのことを万能な人間だと思っている。
  • レビューイの担当タスクについて詳細に理解しているし、プログラミングスキル等も全てにおいてレビューア以上のスキルを有していると思い込んでいる。全てを丸暗記していて、なんでもすんなり思い出して理解できるスーパープレイヤーだと思っている。
  • だから、いちいち言わなくても分かっているはずだろう、明記しなくても意図が読み取れるはずだろう、と思っている。
• 理由や経緯の重要性を考えたことがない。
  • たぶん「丸投げレビュー」する大多数はコレ。自分がやっていることに対する全ての認知が低い。
  • 「何のための仕事なんだっけ？どこに影響するモノなんだっけ？このやり方に問題はないかな？他により良い方法はないだろうか？」こうした考えを漠然としていて、レビューアに同じ認識があるかどうか確認もしなければ、そもそも「自分と他人に情報の非対称性があること」自体を理解していない場合がある。

多分、言葉足らずなレビュー依頼を出すレビューイの多くが、レビューア経験がない人間だと思う。だから、レビューする側にとって必要な情報が認識できないのだと思う。

レビューイから何を伝えるとレビューアが助かるか

レビューイは、レビュー依頼をする時に、

1. レビュー対象に関するタスクのおさらい
2. 作業中に特に考慮した点と、どのように工夫したのか
3. 試行錯誤の結果、採用しなかった手法とその理由
4. 成果物の正しさをどのように確認できるか
5. それ以外に見てほしいと思っているところ

あたりは伝えるようにしよう。

レビューイから伝えて欲しいことの例

「入力された数値のチェックを行う関数を作りました。コードレビューをお願い致します。」

1. タスク概要：
   • このプルリクエストに対応するチケット No. は #2001 です。
   • チケット No. #1500「年齢欄に小数を入力されるとバグる」件で必要になった、入力チェック関数の実装です。
   • 小数・負数を NG とする関数を作りました。
   • この関数を導入する後続タスクは、リリースタイミングの都合上、チケット No. #3905 で実施することになっていますので、本タスクは関数の作成までが作業範囲です。
2. 考慮したポイント・工夫した点
   • 入力値に数値以外の文字列が混じっている場合も考慮しないといけないかどうか、以前議論になりましたが、2018-12-01 開催の会議で「数値形式でない文字列は別の関数でチェックする」と決まったので、文字列の混在チェックは行っていません (相談経緯は議事録を参照)。
   • 浮動小数点の誤差により整数と誤判定することがあったので、予め入力値を □□ メソッドで変換して参照するようにしました。
3. 採用しなかった手法とその理由
   • 数値かどうかの判定に ×× メソッドが使えそうでしたが、JavaScript の言語仕様上、◯◯ という値も数値とみなされるため、△△ メソッドを使用するに至りました (コードコメントにも記載しています)。
4. 成果物の検証方法
   • あわせてコミットしているユニットテストコードのとおり、-1・-0.1・0.1 が全て NG と判断され、0・1・2 などの値が OK と判断されることで、今回作成した関数の正当性を確認できるかと思います。
   • 前述のとおり、文字列の混在チェックはこの関数で行っていないため、'myStr' を引数に渡すと UnexpectedArgumentError 例外が発生します。文字列チェックを行う isValidString() 関数と併用することで、例外を回避して結果を返すケースを作りました。
5. 他に確認してほしいところ
   • 浮動小数点の誤差による誤判定は防ぐよう対応しましたが、他に考慮が必要な特殊な値があればご指摘いただければと思います。

…このぐらいは、レビュー依頼を出すタイミングで一緒に伝えてほしいと思う。

レビューアの思考の変化例

この違いで、レビューアがどのように助かるか、ひいてはどのようにレビューイの労力削減につながるか、Before → After で例を示そう。

1. 「レビュー依頼が来たけど、何の作業が起因なんだったっけ？」
   • → 「あぁ、チケット #1500 のバグ対応のヤツか、これユーザ受け入れテストで見付かったバグだから、ちゃんと直しておかないとお客様に示しつかないから気を付けなきゃな…」
2. 「このタスクでの対応範囲はどこまでなんだっけ？ソース見た感じ、この関数を使う箇所がないんだけど…」
   • → 「そうだった、リリースの兼ね合いでバリデータの適用だけは別タスクに切り出したんだったな。今回は関数単独でレビューするってワケだね」
3. 「関数を見ると、いきなり △△ メソッドでチェック始めてるけど、これって文字列が渡されたらエラーになるよな？文字列チェックは要らないんだっけ？なんかそんな話した記憶がかすかにあるけど…」
   • → 「そうか、あの時の打合せで文字列チェックの関数を手前に入れるって話したっけ。一応その議事録もチェックしておこう。どれどれ…うむ、大丈夫だな。じゃあこの関数は数値チェックだけで良いんだな」
4. 「数値の判定って △△ メソッド使うより ×× メソッド使った方が速くないか？何でわざわざ △△ メソッド使ってこんな複雑な判定してるんだろ…」
   • → 「へぇー、△△ メソッドはこのパターンが上手く拾えないのか。それなら確かに判定が複雑になるけど ×× メソッドを使った方が良さそうだな」
5. 「コードを静的に見た感じは大丈夫そうだけど、境界値チェックとかできてるかな？」
   • → 「ユニットテストコードの解説まで書いてくれてる、なるほどコレだけやれているなら大丈夫だな。」
   • → 「文字列チェックの関数と組み合わせた時のことも認識して、ちゃんと考慮してあるな、よしよし」
6. (ココまで色々と再調査しないといけないことが多すぎて疲れた…。他に確認しないといけないところ…うーん、まぁないんじゃないかな、これだけ調べたら大丈夫だろうきっと…) → (そしてバグ検出漏れへ…)
   • → 「他に確認してほしいところ、か…。あ、待てよ、JavaScript って -0 と +0 があったよな、コレは大丈夫かな…？」
   • → 「もっとあら捜し…。おや、この関数の作りだと、2..59 のようにピリオドが2つ以上入っていた時にバグりそうだな、このパターンで見直すよう指摘を入れておこう」
   • → 「そういや浮動小数点の誤算による誤判定を防ぐロジックは入ってるけど、本当に防げているのかユニットテストしていないな？1.99999999999 みたいな値は大丈夫かな？念のためケースを増やしてもらおう」
   • ⇒ (そして誤判定を防ぐロジックにまだ漏れがあったことが発覚、無事修正できて本番障害を回避)

例としてはデッチ上げたモノだが、レビューアは大抵、こういうことを気にしていて、そこに対する情報が親切にガイドされていると、とてもレビューしやすいし、本質的にレビューしないといけないポイントにより集中できるようになるので、レビューの効果・質が向上するのだ。

レビューイも「レビューアをコントロールする」つもりで

仕事は、現状に対して何らかの変化を与えたくて行うはずだ。

文章は、読み手のその後の行動に何か変化を与えたくて書くはずだ。

レビュー依頼する時の文章をもって、レビューアに何を考えてもらいたいか、レビューアにどうして欲しいのか、伝えよう。レビューアが自分の思ったとおりに動いてくれたら、適切な文章が書けているということだ。書いてあることに関して質問されたり、「どういう意味？」と聞かれるようでは、まだまだレビューアをコントロールできていない。

レビューアにどう思ってほしいか、というのは、「自分の仕事をどのようにチェックしてほしいか」をアピールする、ということなので、やったことをうまく見て評価してもらえるように、必ず理由や経緯をあわせて伝えよう。