コードレビュアーが知っておきたいこと
はじめに
チームでコードレビューを取り入れていると、
「コードレビューを任されたが、どうしたらいいか分からずに
なんとなくでレビューをしてしまった」
「ついキツイ言い方で指摘してしまった」
など、実際にそんな経験をした方も多いかと思います。
「自分の時は厳しく言われたのだから、
レビューして貰えるだけありがたいと思え」のような
あまり良くない考え方をしてしまっている方もいるかもしれません。
（どんなことがあっても八つ当たりは正当化されません）
これは私が一年目の頃のエピソードです。
私が業務で書いたコードを、当時のチームリーダーがレビュアーとして
コードレビューをしてくださったのですが、
その場でチームメンバーを集めて、悪いコードの例として公開処刑のように晒されてしまいました。
「こういう書き方はないよねー」などとメンバーの前で言われ、私はそれから数週間、コードを書くのがメンタル的に億劫になってしまいました。
本人に悪気は無かったのかもしれませんが、私にとってはいまだに嫌な記憶として残っています。
こういったことがあるとレビュイーもレビュアーに不信感を抱いてしまいます。
それに加え、レビュイーのモチベーションおよび生産性の低下も懸念されます。
これはコードレビューが悪いというわけではなく、
コードレビューというものを上手く活用できなかった結果、起こってしまった悲劇です。
このようなことを避けるためにも、
レビュアーが知っておきたいことを考えていきたいと思います。
※コードレビューが有効かどうかについては、この記事では触れませんのであしからず。
レビュアーの役割とは
レビュアーはどんなことをするのでしょうか。
レビュアーはシステムの品質を保障するために、
コードレビューという形で評価を行います。
具体的には、
・仕様や要求を満たせているか
・潜在的な問題が潜んでいないか
・ソースコードの品質が低くないか
などを評価します。
仕様や要求を満たせているか
これが一番重要です。
コードレビューを行う前に、
レビュアーがシステムの仕様を正しく把握しておく必要があります。
仕様書や設計書と整合性が取れていることも、合わせて確認することが大切です。
潜在的な問題が潜んでいないか
稼働したシステムにおいて、すぐには表面化されないバグのことです。
テストである程度潰すことはできますが、
たまにしかその処理が走らないものに多く、発見に時間を要します。
実際の業務であったのが、
・意図していないパラメーターが渡されたときの不具合
・環境差異による動作不良
・うるう年や年次・月次といった発見まで時間を要する処理
などが挙げられます。
環境差異や例外処理といったところをすべて網羅するのは非常に骨が折れ、
どうしても無理なところはありますが、できる限り確認する必要があります。
ソースコードの品質が低くないか
ソースコードの可読性や保守性、再利用性は十分かどうかを確認します。
チーム内で取り決めたコーディングルール等もあるかと思うので、それに準じているかどうかも合わせて見る必要があります。
品質のレビューを行う上で、
デイビット・スコット・バーンスタイン（David Scott Bernstein）氏が CLEAN と名付けた 5 つの観点が参考になります。
Cohesive（凝集性）：特性が明確に定義されているべき
Loosely Coupled（疎結合）：はっきりした責務を担うべき
Encapsulated（カプセル化）：実装は隠蔽されるべき
Assertive（断定的）：オブジェクトの状態は自分自身が管理すべき
Nonredundant（非冗長）オブジェクトの定義は一度だけにすべき
コードレビューで何を伝えるか
コードの指摘としてレビュイーに伝えるべき内容は、
・改善すべき箇所とその理由
・それに対する改善案
この 2 つが守られていれば概ね良いかと思います。
改善すべき箇所とその理由
問題がある箇所を〇〇行目の ✕✕ のように、
なるべく具体的に分かりやすく指摘します。
その際に、なぜその指摘に至ったのか、
仕様書や参考サイトなどで論理的な根拠を添えると良いでしょう。
・2022/07/18 追記
※GitHub の PR やコードレビューツールを使用した場合は、
行単位でのコメントもできるので、上手く活用していただくと良いかと思います。
それに対する改善案
論理的な根拠に基づいた、改善案を提示します。
チーム内において教育という観点から、改善案を提示しないということもあるかと思います。
どのようにレビューをするのか、しっかりとコミュニケーションを取り、ルールとして定めても良いかもしれません。
コードレビューをどう伝えるか
伝え方としては、
・ポジティブな気持ちで取り組む
・否定的な表現は慎重に行う
・ちょっとした一言や絵文字で、印象を柔らかくする
・意思疎通が難しければ、実際にビデオ通話や対面で会話をする
このようなことを意識すると良いかと思います。
テキストコミュニケーションというのは、非常に難しく、
ちょっとした文字の表現で伝わり方がまったく変わってしまうため、
とくに気を付けなければなりません。
たとえば以下は、伝え方のテクニックの 1 つです。
この処理は、パフォーマンスが悪いですが良く書けています。
この処理は、良く書けていますがパフォーマンスが悪いです。
意味としてはまったく同じ指摘ですが、ネガティブな表現を最後に持ってくると
どうしてもネガティブな内容に見えてしまいます。
これはアメリカの心理学者ノーマン・H・アンダーソン（Norman Henry Anderson）氏が、
1976 年に提唱したリーセンシー効果 (Recency Effect)と言って、
「最後に提示された情報が記憶に残りやすい」という現象によるものです。
同じ内容なら、前者の方が良いですよね。
コードレビューの例
上記を踏まえて、コードレビューの良い例と悪い例を比較してみます。
・悪いコードレビューの例
・良いコードレビューの例
まとめ
後者のようなコードレビューの方が、
お互いに気持ちが良いのではないでしょうか。
本来、コードレビューは品質向上（もしくは教育）のために行うものであり、
レビュイーの人格を攻撃するようなコメントは必要ありません。
しかし残念ながら、根拠のある「正しさ」を笠に着ると、
どうしても攻撃的になってしまう人は少なからず存在します。
Google のレビュー指針においても、
礼儀と尊敬が大事であると説かれています。
当たり前のことですが、人と人とのやり取りですので、
お互いに礼儀と尊敬の気持ちを持って接することを大切にして行きたいですね。
参考
Discussion
とても良い記事だともいます！ 書かれてる通り機能要件だけではなく非機能要件の指摘をするの大事ですよねぇ。ところでコードレビュー例で「何行目の〇〇が…」と返信されてる例にされてると思いますが、コードレビューツールやプルリクを活用すれば行単位に指摘が出来るのでお互いに効率的にやれる気がします。端折られただけかもだけど一応。
コメントありがとうございます！
仰る通り、PRやツールを使えば必要はないですね。
一応、Slackとかその他チャットツールでの返信も想定していたので、少し丁寧な内容にしてみました。
なるほどー。自分はチャット使う時も「PRにコメントしといたので確認お願いします」的に返しちゃう事が多いですが、その辺はいろんなやり方がありますものねー。
そうなんですよねー
ちなみに私も同じく「PRにコメントしといたので～」派です。
コメントいただいた内容は追記させていただきました！