はじめに
こんにちは！
友人に光の戦士とやらになれるというオンラインゲームやらない？と誘われているものの、イマイチ踏み込めずに「そのうちやるわ〜」とお茶を濁す日々を過ごしております、まつむらです。
PvP（Player vs Player）よりはPvE（Player vs Enemy）派なので、興味自体はあるんですけどね、、、
一人で黙々と高難易度のクエストを進めるのもいいけど、チームで共闘する方が達成感ありません？
さて、そんなわけで、エンジニアがやるチームプレーの最たるものといえば、、
そう、コードレビューです。
個人としての質とスピードが達成できていたとしても、チーム開発は文字通りチームプレイ。
プルリクエスト（以下PR）作成までが速かったとしても、PRが放置されているのだとしたら、開発速度という点ではボトルネックになりうる。。。
とはいえ、好きでPRを放置しているわけではなく、放置される原因・理由もあるのではないかと。
そこで、PRを放置しないでレビュー速度を上げるために「チーム内で取り入れているルール」と「個人的に意識していること」を改めて振り返っていこうかなと思います！
チーム内で取り入れているルール
1つのPRにて変更行数は500行以内、変更ファイル数は20ファイル以内にする
日本語訳版が発売されてからだいぶ日が経ちましたが、「Looks Good To Me」という本にも書かれている変更上限をルールとして設けました。 ※Looks Good To Meに関しては、kogureさんがまとめてくれているので、興味がある方はこちらもチェック！ tech.iimon.co.jp
- 変更行数は500行以内
- 変更ファイル数は20ファイル以内
体感でしか測れていませんが、だいぶ見やすくなったように感じてます。
少なくとも、タスクで必要なものとあったら嬉しいもの（ついでの修正、リファクタリング、ツールの追加）でPRは分けやすくなったのかなと。
また、レビューにて確認する行数が減ることにより、
「物量すごくて何だか分からんがヨシ！」
「前の人がヨシと言っているのでヨシ！」
「前の前の人がヨシと言っているのでヨシ！」
と流し見されてしまうPRはなくなるのかなと思います。
PRのテンプレート設定とチケットのリンク
PRに何を書いていいのか分からない！といったこと、あると思います。
これに関しては方針によってまちまちだとは思いますが、チーム内では以下を記入するようにしています。
- チケット
- 概要
- 対応
- 動作確認
- チェックして欲しい箇所
- 共有事項
チケット
これに関しては、説明するまでもありませんが、チケットのURLを貼っています。 弊社ではNotionを利用しているので、PRタイトルにチケットのIDを付けると自動でリンクされるようにはなっているのですが、念のため貼るようにしてもらっています。
概要
PRの概要を書きます。 こういう事象があったからこういうことが必要になった、と言った感じですね。 概要なので、全体像が掴めればおｋとしてます。
対応
どういう対応をしたのかを書きます。
個人的にはチケット内の工数表に合わせてチェックボックスを置いたりしてます！
とはいえ、チケットに対応内容を記載することも多いので、「Notion参照」をデフォルトで置いてます。
動作確認
画面のキャプチャや、どう言った確認を行ったのかを書きます。
ここも対応と同じく、チケットに動作確認を記載することも多いので、「Notion参照」をデフォルトで置いてます。
チェックして欲しい箇所
「ここ初めて対応したんだけど大丈夫かな、、、」 「関連する箇所が多そうだけど、全部網羅できているかな、、、」 といったチェックして欲しい箇所書きます。
不安を吐き出すことで、心理的安全性を保つ意味合いが大きいのかなと！
共有事項
レビュアーに伝えておきたいことを書きます。
「こういうUtil追加したよ！」とか、「こういう使い方は非推奨にしていきたい！」、「他メンバーのタスクと連携するよ！」などですね。
定例でも共有時間は設けていますが、善は急げ！とスピード感をもって連携していきたいのでこの項目を設けました。
特にない場合は丸ごと削っちゃったりもします。
さて、PRのテンプレートに関しては以上なのですが、これが最適解かというと永遠の課題ですね、、ただ、今のところ、この6項目に落ち着いてます。
また、これらはPR作成時に自動で読み込まれるようにしているので、「どういう項目を書けばいいんだっけ、、、？」と思い出しながら記載するコストも削減してます。
PR作成後のセルフレビュー
セルフレビューと言ったものの、厳密には突っ込まれそうな箇所に先んじてコメントしておく、と言った方が正しいかもしれないですね。
レビュイーは完全に理解していることでも、レビュアーは理解しているとは限らないので、複雑な処理などは補足としてコメントをするようにしています。
会話のキャッチボールは大事ですが、レビューに関しては往復が少ない方が速度が上がるのは間違いないのかなと。
また、弊社ではAIレビューツールを導入しているため、ドメイン知識が不要な箇所に関しては自動でレビューされるようになっています。
コーディング規約も読み込ませているので、それに合わせて抜け漏れがないかもチェックしてもらってます！
AIにコメントされた箇所に関しては、修正対応or対応不要な理由を記してもらうことで、前もってレビュアーの負荷を下げられるようにしています。
なので、レビュアーはドメイン知識が絡む箇所を中心に確認すれば良いように負荷軽減はできているのかなと！
もちろん、過信せずに拾ってく姿勢は持たなきゃですが、それでもフルで見るのと比べるとだいぶ楽にはなりました！
朝会後のレビューRTA
これはkogureさんのチームが導入していて良さそうだなと思って導入した仕組みです！
進捗状況、共有事項、困り事がないか、などの相談をする時間として朝会を行なっているのですが、朝会終了後にそのままレビュー合戦を行うようにしています。
レビューが終わった人から抜けていくという方式なので、基本的にレビューが終わらない限り、自分のタスクへは進めないようになっています。
終わるまで抜け出せないのはちょっとやりすぎかな、、、と思って1週間だけのトライアル導入ではあったのですが、
「集中してレビューする時間が取れる」
「レビュー忘れがちだったのでちょうどいい」
という声もあったので、今日まで続いております。
また、朝会後そのまま実施、つまりコアタイム中のレビューになるので、「質問したけど帰宅しちゃったから返事は翌日になっちゃうかな、、」といった時間の問題も解消できているのかなと思います。
夜行性エンジニアにも優しい！
個人的に意識していること
さて、ここからは完全に「個人的に意識していること」という自分語りにはなってしまうのですが、チームとして強制することではないのかなと言った内容になります。
心理的安全性を担保する
ラベルをつける
正直チーム単位で導入してもいいのかなとは思いますが、ラベルの定義をしていないからというのがあるため、個人的な意識付けで止まっているところもあったり。。。
レビューとなると、どうしてもPvPという意識が強いため、否定されたり攻撃されているように感じてしまうこともあるのかなと思います。
「この質問の意図は何だろう、、？」「この書き方じゃダメなの、、？」「もしかして嫌われてる、、？」と裏を取ろうと思えばいくらでもその深みへとハマっていってしまう、、、
ただ、コードレビューはPvPではなくPvE、協力して敵（悪いコード）を倒していくのがコードレビューなので、変な誤解を与えないようにコメントの仕方はよく考えなきゃだなと思うわけです。
そのために、個人的に以下のラベルを付与するようにしています。
- Q - Question
気になったから聞いてみたよ！といった意図で付けてます。 基本的にはこのラベルをつけたからには、同一コメント内での提案はしないようにしてます。 タイポかも？と言った時もこのラベルをつけがちかも？
- IMO - In My Opinion
私が思うに、、、といった自分だったらこうするかな？といった意図で付けてます。 こうした方がいいかも？というあくまで提案ベースではあるので、上記のQ+提案といったかたちですかね。
- NITS - Nitpick
動作自体には影響しないけど、ちょっと気になる！といった細かいことかもだけどコメントしたよ！といった意図で付けてます。 早期リターンしてネスト浅くできそう！だったり、相対パスが深すぎるからエイリアス使った方がいいかも？といった感じですね。
- MUST
これは修正必須なので直してください！といった意図で付けてます。 ただ、ここ最近はテストコードが充実してきたので、付ける機会は減ったかもしれませんね。
正直、IMOとNITSの使い分けが曖昧にはなっているので、"チームとして浸透させるには"という点で考えた場合、ちゃんと定義した方が良さそうですね、、！
コメントの内容に気を付ける
過激なコメントにならないように日頃から意識はしているのですが、その中でも「伝わるコードレビュー」にて心理的安全性を担保するためのコメントの仕方について分かりやすく言語化されていたので紹介します！
クイズを出さない
「このメソッドを使うのは良くないんだけど、理由はわかる？」といったコメントだと前述のPvPに発展してしまうので、クイズ形式での質問はNGかなと。
クイズを出すのではなく、相手にして欲しいことを素直に伝える方が良いです。
例えば、Promise.allで処理できるところをループで1つずつ処理している箇所を指摘するとして、
「毎回awaitするの良くないんだけど、理由はわかる？」と圧を感じさせるよりは「promise.allの方がまとめて処理できてお得だよ！」と、逆に理由を添えて導いてあげる方が優しいのかなと。
実際、レビュー中にこういう場面に対峙した場合、「こういう意図で合ってる？そうであれば、このutil使えるよ！」とPermalink付きでコメントすることが多いです。
何処にどういった処理があるのか分かれば、次からはそこを勉強すれば解決の糸口が見えてくるので、お互いwin-winなのかなと！
命令しない
相手の意思や状況を考慮せずに強制的に指示を出すのはNG。
「ここはこうやってください」と一方的に命令するのではなく「こういう懸念点があるので、ここではこうしてください。」と理由を添えてあげるのが吉。
弊社ではエンジニア行動指針 - Code is boss - にて「誰が書いたかに関係なく議論して、より良くしていこう！」というものがあるので、そもそもあまり起こり得ないものではあるのですが、素敵な考えだなと思ったので、引用させていただきました！
「なぜそうしたのか」その背景を聞いてみる、もしくは「こういう理由からこうした方がいいと思ったけど、どう思う？」とフィードバックを求めるようにする方が建設的です。
その際も、クイズ形式にならないように注意したいですね、、、！
遠回しに言わない
気を遣って結構やりがちなので気をつけなきゃではあるのですが、遠回しに伝えようとすると、1/3も伝わりません。
日常会話だったり、処世術としては気を遣って遠回しに言うのはアリかもですが、レビューに関しては避けた方が良さそうです。
というのも、レビュアーは遠回しな表現にエンコードするコストが、レビュイーは個人の匙加減でデコードするコストがかかります。
そうして1つ変換を噛ませた結果、遠回しだと本当に伝えたいことが伝わらないので、やり取りが増えてしまうというデメリットも存在します。
結局、「何を伝えたいのか？」という点でコメントするのが良いのかなと思います。
もちろん、その場合でもほんわかコメント推奨で！相手の立場になってコメントを意識するのが大切ですね！
「念のため」の確認をする
これは割と良くやるのですが、「念のため」と「ちなみに」は魔法の言葉なのかなって思ってます笑
ワンクッション確認を取りたいといった時に使うと、「念のため確認したいだけなんだな」という安心感が生まれるのかなと。
……と、ここで記事にしちゃうと、かえって深読みされちゃうかもですが、本当にそういう意図はないので安心してくださいね（？）
論破ではなく納得を目指す
レビュアーとレビュイーで意見が割れた時に、どちらが正しいのかと対立しそうになることも少なくはないのかなと思います。
ただ、これに関しては明確な答えがない場合も多い（方針・開発スタイルによりけり）ので、意見をぶつけるのは大事なことですが、言い争いはNGです。
お互いのメリット・デメリットを共有して、その上でどちらに舵を切るのかというのが重要なのかなと。
時には周りを巻き込んで「2通りの方法があって、それぞれメリット・デメリットはあるんだけど、どっちの方がしっくりきますか？」と意見を募るのも手なのかなと。
その際、「正しいか」ではなく「どっちがしっくりくるか」が重要なので、各々の意見も出してもらった方が、納得感ある状態でゴールに着地できます！
おわりに
今回はコードレビューについて「チームとして実践していること」と「個人的に意識していること」について書いてみました！
対戦相手は人ではなく、あくまでコード。
より良いコードにするためのチームプレー。
PvPではなくPvEであるということが伝わりましたかね？
まだまだ改善の余地あり、仕組みづくりで解決できそうな点は多々あるので、これからもメンバーの意見を拾いながら模索して、よりコードレビューをしやすい環境に持っていければと思います！
最後まで読んでくださりありがとうございます！
この記事を読んで興味を持って下さった方がいらっしゃればカジュアルにお話させていただきたく、是非ご応募をお願いします。
iimon採用サイト / Wantedly / Green
参考資料
コードレビューにラベルを付けるだけでチームの心理的安全性を高めた話
Looks Good To Me - 秀和システム新社 あなたの学びをサポート！
伝わるコードレビュー 開発チームの生産性を高める「上手な伝え方」の教科書（鳥井 雪 久保 優子 諸永 彩夏 島田 浩二）｜翔泳社の本
