アンチパターンから学ぶHRTなコードレビュー

(image)アンチパターンから学ぶHRTなコードレビュー
目次

『Team Geek』の書評でも書いたんだけど、コードレビューのときはHRTの精神を大事にしたい。

What is HRT?

HRTとは下記の３つの精神のことだ。

  • Humility（謙虚）
  • Respect（尊敬）
  • Trust（信頼）

HRTについて詳しくは弊ブログの下記記事を参照。

以下、アンチパターンなコードレビューのシーンを先輩（👿）と後輩（👼）でお送りする。

アンチパターン1・コメントが怖い

⛔️アンチパターンなコードレビュー

👿「typoです。直してください」

👿「これはこう書くべきです（…以下コード提案…）」

👿「どうしてこのようなコードにしたんですか？」

👼「（えっ… 怒っているの？ 怖い…）」

説明

コードレビューじゃなくとも、テキスト・コミュニケーションであれば手段を問わず発生しうる問題。文字だけのコミュニケーションは、発言の背後にあるコンテキストや感情が汲み取りにくい。

上記の例だと背後にある感情が「typoじぇねぇか💢 直せ😠」なのか「typoみつけちゃった👀 指摘して直してもらおう😃」なのかがわからないので、先輩・後輩という関係性を前提とした場合、後輩からは先輩が怒っていると思われていても仕方ない状況と言える。

先輩の他の指摘に関しても同様に、「こう書くべき💢」なのか「個人的にこう書くべきだと思っているんだけどなぁ…🤔」なのかわからないし、「どうしてこんなひどいコード書いた？💢」なのか「このコードの意図が知りたいなぁ、どうしてこう書いたんだろう？🤔」なのかがわからない。

解決策

  • 絵文字を使う😃
    • 絵文字で感情を表現して、発言の背後にある感情の誤読を減らす
  • コードレビュー・ラベルを使う
  • 断定口調・命令口調ではなく提案口調を使う
    • 「〜すべき」「〜してください」などの高圧的な断定・命令口調ではなく、「〜どうでしょう？」などの提案口調を使う
  • 質問の背景を明確にする
    • 質問をするということは質問をするに至った背景が何かあるはずなので、それを一緒に提示する
  • 口頭で指摘する
  • 上述の[nits]な指摘で自動化できる点は自動化する
    • Rubyにおけるrubocop、JSにおけるESLintなどは自動化可能なのでCIで自動化する
    • 人間がいちいち指摘するよりも時間的コスト・心理的コストがレビュワー、レビュイー双方にとってずっと低い
    • ツールとしてはreviewdogやsiderなど

✅改善後

👿「[nits]typo❗️😄」

👿「[imo]これはこう書くべきだと思ったのですが、どうでしょう❓😺（…以下コード提案…）」

👿「[ask]モデルにあっても良い処理なように感じましたが、どうしてここに処理を書いたんでしょう？」

👿「[ask]xxx関数を使えばもうちょっとシンプルに書ける気がしますが、どうしてこう書いたんでしょう？」

🤖(bots) 「Trailing whitespace detected.」

あるいは口頭で:

👿「さっき上げたコードでちょっと気になるところがあるから聞いてもいいですか？ （コードを指差しして）この部分のコードの意図が汲み取れなかったんですけど、どうしてこんなコードにしたんでしょう？」

👼「あー、そこは〇〇〇で、△△△だからそう書いたんですよね。（…以下説明…）」

アンチパターン2・理由がない

⛔️アンチパターンなコードレビュー

👿「これは残念なコードですので、直してください」

👿「この書き方をしたらダメです」

👼「はい…（えぇぇ！？ 何がダメなんだろう…？）」

説明

コードレビューコメントに指示はあるが、理由がないケース。

上記のケースだと「なぜ残念なコードと言えるのか？」というポイントが欠如している。コードに対する価値観が完全に合致していれば、「残念なコード」でレビュイーに伝わるが、大抵の場合はそうではないので理由がないと伝わらないケースが多い。特にレビュイーがジュニアレベルであればコード良し悪しの分別がつかないのでなおさら伝わらない。

解決策

  • レビュイーが納得できるに足る理由を示す
    • なぜ残念なのか？ なぜダメなのか？ 必ずWhyを示す
    • 「個人的にこっちのほうが好きだから」「なんとなく」などは納得できる理由ではないのでアウト 👉 上述の[imo]提案行き

✅改善後

👿「これはマジックナンバーになっているので、定数に切り出すように直してください😉」

👿「この書き方はXSS脆弱性がある書き方だからダメです🙅」

👿「[imo]こっちのほうがスッキリしていて個人的に好きなのですがいかがでしょう？（…以下コード提案…）」

アンチパターン3・誹謗中傷

⛔️アンチパターンなコードレビュー

👿「これはクソコードですね」

👼「ごめんなさい（一生懸命書いたのに…（泣））」

説明

コードに対する誹謗中傷。これを乱用すると現場が殺伐とする。

解決策

  • 「クソコード」などのコードの誹謗中傷となるような言い回しは使わない
    • 「あまり強い言葉を遣うなよ。弱く見えるぞ。」
  • 口頭でコミュニケーションする
    • クソコードのような強い言葉を使いたくなるような場面は感情が昂ぶっているときであり、テキスト・コミュニケーションだとすれ違いが起きる可能性が高い。クソコードと思い至るまでの理由を膝を突き合わせてきちんと説明する

✅改善後

👿「これは今後メンテしていくのがとても辛いコードになりそうです。なぜなら〇〇だし、△△ともなり、××だからです。」

👼「なるほど〜〜〜！（私クソコードを書いてたかもしれない…）」

アンチパターン４・大量コメント

⛔️アンチパターンなコードレビュー

👿「〇〇〇〇」

👿「△△△△」

👿「××××」

（以下大量のレビューコメント）

説明

一連のコード変更に対して大量のレビューコメントが付く事象のこと。

シニアエンジニアがジュニアエンジニアのコードをレビューするときなど、レビュワー・レビュイー間のスキル・知識・経験に大きな差があるときに陥りやすい。大量のコメントを付ける/付けられることになるので、レビュワーにとってもレビュイーにとっても心的労力が高い。

また１つ１つのコメントがどれだけ丁寧だとしても、その数が多くなるとレビュイーに無力感を味あわせてしまうことになり心理的ストレスに繋がる可能性がある。

解決策

  • コードの差分を小さくするように分割する
    • コメントが多く付くのはそもそもコードの差分が大きいからなので差分を小さくしてレビュー対象コードを少なくする
    • GitHub であれば Pull Request を可能な限り分割し１つ１つを小さい差分に保つ
  • コードレビューの代わりにペアプログラミングをする
    • コードレビューが大変ならコードレビューをしない。ペアプロでその場で一緒に解決させる
    • モブプログラミングでもOK
  • 研修の実施および内容見直し
    • スキルに大きな差があることが原因なので、その差を埋めるための研修を実施
    • もし新入社員向けに研修がないのであれば、新入社員を対象に技術研修を新たに計画すべきかもしれない。既にあるのだとすれば研修内容の見直しが必要かもしれない
    • 中途であればオンボーディング・プロセスの見直しが必要かもしれない

✅改善後

（ナビゲーター👿/ドライバー👼がペアプログラミング中）

👿「ここのコードは 〇〇〇 できれいに書けるんじゃないかな」

👼「なるほど、やってみます。（コードを修正する）」

👿「次に 〇〇〇 によって ××× が要らなくなったから直せそう」

👼「あ、たしかに。やってみます（コードを修正する）」

アンチパターン５・長大な議論

⛔️アンチパターンなコードレビュー

👿「ここの設計、私としては 〇〇〇 だと思うんです」

👨「僕はそうは思いません。△△△ にしたほうが良いと思うんです」

👩「私は 〇〇〇 案と △△△ 案を組み合わせた ××× でも良いかと」

（以下、👿, 👨, 👩 の議論が延々と続く）

👼「（あわわわわわ、、、どうすれば…）」

説明

１つのコードレビューに端を発して長大な議論をスタートさせること。これが始まると一つのコメントに対してのスレッドがどんどん伸びていくことになる。

設計などの大枠なトピックだと、長大な議論になりやすい。

解決策

  • 話し合いで解決させる
    • テキストで延々と話すより、わーっとみんなで集まって話し合うとすぐに決まるケースが多かったりする
    • 話し合った結果をテキストとして残しておく

✅改善後

👿「ここの設計、私としては 〇〇〇 だと思うんです」

👨「僕はそうは思いません。△△△ にしたほうが良いと思うんです」

👩「私は 〇〇〇 案と △△△ 案を組み合わせた ××× でも良いかと」

👿「では話し合って決めましょう！」

（集まって話し合い）

👿 👨 👩「議論した結果、△△△ が良いとなったので △△△ な方針でいきましょう」

👼「ラジャー!」

なぜHRTなレビューが必要なのか？

なぜHRTなコードレビューを心がける必要があるのか？　答えはチームの心理的安全性を確保するためだ。

全ては心理的安全性のため

最近では共通認識になりつつあるので改めて言う必要はないかもしれないが、ハイパフォーマンスなチームにとって心理的安全性は極めて重要だ。それはGoogleの調査によって証明されている。

Google のリサーチチームが発見した、チームの効果性が高いチームに固有の 5 つの力学のうち、圧倒的に重要なのが心理的安全性です。リサーチ結果によると、心理的安全性の高いチームのメンバーは、Google からの離職率が低く、他のチームメンバーが発案した多様なアイデアをうまく利用することができ、収益性が高く、「効果的に働く」とマネージャーから評価される機会が 2 倍多い、という特徴がありました。

via. Google re:Work - ガイド: 「効果的なチームとは何か」を知る

psychological safety

モヒカン族の方々は下記のように考えるかもしれない。

  • 「何を言ったってそれはコードに対する批判であって書いた人への批判ではない。何を言ってもええやろ」
  • 「間違ったことは言っていない。言い方はどうだってええやろ」

気持ちはわからなくはないが、残念ながら我々は人間である。人間であるということは感情があるということ。自分が頑張って書いたコードを「クソコード」などというリスペクトの無い言い方で批判されて平常心でいられるだろうか？　「〜するべき」「〜しろ」などと高圧的なコメントを受けてレビュイーは気持ちよく受け止められるだろうか？　そこに違和感があった場合、遠慮なく異を唱えることができるだろうか？

「心理的安全性を気にしすぎて何も言えない」のではなく、お互いが年齢・ジェンダー・人種の区別なくフラットな関係性で何でも言い合えるようにするためのHRTであり、そのための心理的安全性である。そこは勘違いしてはいけない。

チーム中にある個人の関係において、「様々な形で課題や問題についての提起がされる」ということに他なりません。（…中略…）つまるところ、心理的安全性が高いとは、「些細な問題であっても提起される」「多く問題に対して自己主張がなされる」という観測可能なチームの状態を意味しています。

via 心理的安全性ガイドライン（あるいは権威勾配に関する一考察）

参考情報

関連記事