こんにちは！今回も『伝わるコードレビュー 開発チームの生産性を高める「上手な伝え方」の教科書』の感想文を書いていきます。
「Part1 心構え編」については、以下の記事をご参照ください！
はじめに
「『伝わるコードレビュー 開発チームの生産性を高める「上手な伝え方」の教科書』~実践編~ 」では、19個のケースが紹介されています。
本記事では、Case1~10までです。
Case1: 緊張感のあるレビューコメント
まずは、プルリクエスト内で以下のようなコメントがされた場合についてです。
レビュアーから「なんでこのメソッド使ったの？」ってコメントが来たら、皆さんはどう思われますか？大体の人は、「怒られてる...？」って思いますよね。
このレビューの問題は以下です。
- コメントが言葉足らずで質問の意図が伝えきれていない
- レビュイーが深読みしすぎて、コミュニケーションが止まる
1. コメントが言葉足らずで質問の意図が伝えきれていない
今回のケースでは、レビュアーは質問をしています。しかし、レビュイーは指摘として捉えています。ここで、認識がずれていますよね。
レビュアーは、このコメントが質問なのか指摘なのかをはっきりとさせる必要があります。そして、質問である場合には、質問の意図や批判ではないことをはっきりと伝えておく必要があります。
これを踏まえると、レビューコメントは以下のように改善されるのではないでしょうか。
ポイントは以下です。
- [ASK]のように、コメントの意図を明確にするタグを利用する
- 質問の背景、理由を明確にする
- 批判ではないことを明確にする
- 何を答えて欲しいのかを明確にする
2. レビュイーが深読みしすぎて、コミュニケーションが止まる
今回のケースでは、質問の意図が明確になっていないため、レビュイーはコメントされた内容以上に何か指摘されているのではないかと考えてしまうこともあると思われます。
コメントの内容以上のことを考えてしまうことは、コミュニケーションの遅延を引き起こしかねません。レビュイーは、コメントされた内容以上のことは読み取らないという訓練をする必要があります。
もし、コメントの意図に悩んだ場合は、以下のようにレビュアーに質問をしましょう。
また、文面でのコミュニケーションの中で「行間を読む」ことが大事と言われますが、これは「文章には直接表現されていない筆者の真意をくみとる」ということです。難しいんですよ。LINEでのやり取りでもそんなことできませんよね。
そんな高度なテクニックを使うくらいなら、「行間を読まなくても情報が出揃うコミュニケーション文化を作る」ことを意識するべきです。
ポイントは以下です。
- 文章に書かれていること以上の内容を読み取らない
- 質問の意図がわからなかったら、わからなかったことを伝える
Case2: 説明不足のプルリクエスト
次は、以下のようなプルリクエストが送信された場合についてです。
このプルリクエストでは、どのような問題が発生するでしょうか。
- プルリクエストのディスクリプションにレビュイーから十分な情報が提供されていない
これに尽きるでしょう。
1. プルリクエストのディスクリプションにレビュイーから十分な情報が提供されていない
レビュアーがこのプルリクエストを見た時に、何をレビューしたら良いのかわかるでしょうか？そもそも、「コードを見て何を実装したのか」を確認するところから始めなければいけないですよね。
この状況はレビュアーにとって非常に負担になります。その状況を回避するためには、プルリクエストのディスクリプションに十分な情報を記載する必要があります。
具体的には、以下の情報を記載すると良いのではないでしょうか。
- 概要・・・実装/修正した機能
- 動作確認・・・どのように動作確認をすれば良いか
- 気になっているところ
- 参考リンク
これを踏まえると、プルリクエストは以下のように改善されるでしょう。
最低限、概要と動作確認方法が明記されていれば、何が修正されたのかがパッと見て理解できますね。
また、プルリクエストのディスクリプションのテンプレートを用意しておくことで、レビュイーが何を記載すれば良いのか考える時間を削減することができます。
ポイントは以下です。
- ディスクリプションのテンプレートを作成し、情報の漏れを防ぐ
- 実装時に悩んだ点や判断した理由などの背景情報も記載する
Case3: 進捗が遅れているPR
開発を進めている中で、複雑な機能の実装でコードレビューはしっかりとしたいけど、期限が短く、すぐにマージしたいというケースがありませんか？
こういうケースでは、以下のような問題があったりします。
- レビュアーが期限を知っている前提にしている
- チームメンバーが期限を把握できていない
- コメントのやり取りだけで進めている
1. レビュアーが期限を知っている前提にしている
レビュイーは期限を把握した上で実装を進めているわけですから、期限を知っているのは当然なのですが、レビュアーはプルリクエストを受け取った時点でその実装内容の存在を知ることもあるので、期限を把握できていない場合もあります。
そのため、作成したプルリクエストが急ぎのものである場合、レビュアーが期限を知っていたとしても知らなかったとしてもプルリクエストが急ぎである旨は共有するべきです。
ポイントは以下です。
- レビュアーがプルリクエストの期限や状況を知っている前提にしない
2. チームメンバーが期限を把握できていない
レビュイーがレビュアーに期限を共有していないことも問題ですが、レビュアー自身が期限の確認を怠ってしまうことも問題です。また、レビュアー自身が期限を確認しづらい環境であることも問題ですよね。
このような問題は、「期限を可視化すること」「期限を把握できるような仕組みを作ること」で解決できます。例えば、以下のような施策が考えられます。
- プルリクエストのテンプレートに期限を記載する箇所を設ける
- プルリクエストのラベルで期限を明示する
- 朝会などで定期的に状況を確認し合う
上記のような取り組みによって、期限の確認漏れ・共有漏れが防げます。また、コミュニケーションのスピードも上がり、問題の早期発見や迅速な対応に繋げることができます。
また、予定通りに進まなかったタスクについては、チームで振り返りの機会を設定するのも良いのではないでしょうか。
ポイントは以下です。
- 見える仕組みを作って確認コストを減らす
3. コメントのやり取りだけで進めている
プルリクエストのレビューは、基本的にコメントでのやり取り（非同期）ですが、急ぎのものである場合には会議を設定してレビューを行うこと（同期的）も良い方法です。また、進捗が遅れそうな場合には、すぐに共有して調整が可能か確認をする必要があります。
ポイントは以下です。
- レビュアーに協力してもらい、同期的なコミュニケーションを取る
- 進捗が遅れそうな時は早めに共有する
Case4: 考え方・価値観の食い違い
例えば、プルリクエストで以下のようなコードが上がったとします。
<% form_for @user |f| %>
<%= f.label :user, :company %>
<% if @current_user.admin? %>
<%= f.select :user_id, Company.all.map { |company| ["#{company.name}（#{company.business_type}）", company.id] }, { include_blank: true } %>
<% else %>
<%= f.select :user_id, Company.all.map { |company| [company.name, company.id] }, { include_blank: true } %>
<% end %>
<% end %>
そして、以下のようなやり取りが行われたとします。
レビュアーAさんは、「このコードは1行で書かなければならない」と考えているようです。一方で、レビュアーBさんは、「このままIF文を使った方がわかりやすいからこのままの方が良い」と考えているようです。どちらも間違ったことは言っていないですね。
ちなみに、1行でまとめるとこうです。
<% form_for @user |f| %>
<%= f.label :user, :company %>
<%= f.select :user_id, Company.all.map { |company| [@current_user.admin? "#{company.name}（#{company.business_type}）" : company.name, company.id] }, { include_blank: true } %>
<% end %>
このやり取りでの問題点は以下です。
- 「正しさ」に対する思い込みが状況に合わない
- 持論に固執しすぎて、相手の意見を受け入れられない
1. 「正しさ」に対する思い込みが状況に合わない
まず、大前提として、コーディングや設計に完璧な正解は存在しません。自分が正しいと思っていることは、その時の状況などによって最良の方法ではないことがたくさんあります。そのため、柔軟な姿勢を持つことが重要です。
一方で、プロジェクト内で一貫性が求められるルールは、コーディング規約として定めておくと良いです。（RubyだとRubocopなど） ただし、コーディング規約自体も状況によっては見直して良いものであることは理解して運用する必要があります。
ポイントは以下です。
- 自分の「正しい」が誰にとっても「正しい」とは限らないことを受け入れる
- 「チームとしての動きやすさ」を踏まえたチーム内のルールを話し合う
2. 持論に固執しすぎて、相手の意見を受け入れられない
自分の意見や提案が、受け入れられないと傷つくこともあります。しかし、コードレビューは議論の場なので、価値観を押し付けるのではなく、相手の意見に柔軟に耳を傾ける姿勢が必要です。
チームには、経験やスキルレベル、コードの書き方の好みが異なるメンバーが集まっています。それぞれのメンバーの基準や考え方が違うのは当然であることを理解し、相手の考えを尊重することで、よりスムーズなコミュニケーションを行うことができます。
上記を踏まえた上で、やり取りを改善すると以下のようになります。
また、コードレビューは対戦プレイではなく協力プレイなので、相手の意見を尊重することで建設的なコミュニケーションが実現でき、ベストな成果へ繋げることができます。
ポイントは以下です。
- 意見を言う時は相手の否定ではなく、対話として提示する
- 相手の視点でも考え、意見に耳を傾ける
- 色々な意見を聞くことで自分の考えの幅を広げる
Case5: 細かすぎるレビューコメント
プルリクエストの中で、以下のようにコメントがされていたとします。
コードの内容ではなくて、形式についての細かい指摘ですね。
ここでの問題点は以下です。
- 明文化・共有されていないルールに基づいたルールに基づいてコメントしている
- 自動化できるチェック項目をレビュアーが指摘している
1. 明文化・共有されていないルールに基づいたルールに基づいてコメントしている
プロジェクトには、一般的にルールがあります。しかし、コーディング規約などの細かいルールは意外と明文化されていなかったり、既存メンバーも言語化していなかったりするものです。これによって、余計なコミュニケーションが発生したり、イライラなどが溜まっていきます。
そのためにも、プロジェクトのルールはいつでも見れる場所に置いておくことが必要です。例えば、以下に記載しておくなどです。
- プロジェクト管理ツール（Github, Backlog, RedMineなど）のwiki
- README
上記のような開発メンバーが普段から意識しやすい場所にルールを明文化しておけば、新規メンバーにとってもルールをキャッチアップしやすくなります。
また、このようなルールを作成した場合、ルールを作ったままにするのではなく、メンテナンスを続けることが必要です。
さらに、ルールを作成するにあたって、気をつけたいアンチパターンがあります。
- 完成しないルール
- 最初に全てをルール化しようとして書ききれず、頓挫してしまう
- 細かすぎるルール
- 誰も厳密なチェックができず、そのうち形だけのルールになってしまう
上記のようにならないためにも、最初に始めるのは最小限のルールが良いです。最小限のルールから始めることで、メンバーの認識を深めやすくなっていきます。また、メンテナンスもしやすくなるため、メンテナンスサイクルの中でチームの文化を育てることができます。
↓メンテナンスサイクル↓
ポイントは以下です。
- プロジェクトごとのルールは、明文化して参照しやすい場所に置く
- ルールは都度メンテナンスし、チームの文化として育てていく
2. 自動化できるチェック項目をレビュアーが指摘している
細かい形式のチェックを機械で実施できるのであれば、その方が工数がかからないし、楽です。それに、他人から指摘を受ける時は相手の感情などを気にしながらコミュニケーションしなきゃいけないですが、機械からの指摘であればそんなこと気にしなくて良いので、楽です。
Gitにはpre-commitという機能があるので、ローカル環境ではこれを使うのが良いかもしれないですね。また、Github ActionsやAWS Codebuild、Jenkinsなどを使用することでプルリクエストを作成した時やブランチにプッシュした時にチェックを行うことも可能です。
ポイントは以下です。
- 機械的にチェックできる項目のチェックは自動化する
- チームのルールーと一緒にツールの設定を育てていく
Case6: LINTツールの言いなりの修正
コードを記述してプルリクを送信する前やプルリク送信時、プッシュ時にコードを静的解析するツール（Rubocopなど）を実行している場合、「警告が出ているからその通りにしなきゃ」と思って修正しますよね。しかし、それではコードの可読性が下がってしまったりする場合があるため、警告が出ているからといって絶対に直す必要があるとは限りません。
今回のケースにおける問題点は以下です。
- Lintツールの警告をクリアすることしか考えていない
1. Lintツールの警告をクリアすることしか考えていない
Lintツールは、チームで統一されたコーディングスタイルを実現するための有効な手段です。コレを利用することで可読性が上がり、品質を向上させることができます。
しかし、Lintツールに従っているだけでは、逆に可読性が下がってしまうこともあるため、その都度判断が必要になります。このような場合、チーム内での対応は以下のような選択肢があります。
- 現状のルールを維持し、必要な個所で個別に無効化する
- チームのコードベースに合わせて上限値を調整する
- ルール自体をオフにしてレビュー時に人の目で判断する
Lintツールのルールファイルに上限値を設定している理由を記載したり、ルールを無効化しているコードにコメントで無効化している理由を記述しておくと、チーム内でコードの理解がスムーズですね。
ポイントは以下です。
- Lintツールの警告はコードの文脈を考慮して反映するかどうか判断する
- チームの状況に合わせてLintツールの調整や見直しを定期的に実施する
Case7: 「あの人が言っているから大丈夫」という思考停止
送信したプルリクエストにて、以下のコードがプッシュされていたとします。
if num.zero?
# numが`0`の時の処理
elsif num > 0
# numが`0`より大きい時の処理
else
# numが`0`より小さい時の処理
end
そして、このコードに対して、以下のようなやり取りがされていたとします。
このコメントを受けた時に、真っ先に言われた通りに修正するのはよくないです。「本当に0が返ってくることはないのか」を確認すべきですよね。
今回のケースにおける問題点は以下です。
- レビュアーが動作を未検証のままコメントしている
- レビュイーがレビューコメントを鵜呑みにしてそのまま提出した
1. レビュアーが動作を未検証のままコメントしている
レビュアーが未検証のままコードに対してコメントを残すことは無責任なコメントとなってしまいます。より良くすることができると感じた場合でも、手元の環境で検証を行ってからコメントを残すべきでしょう。
ポイントは以下です。
- 自分を信じすぎず、動作確認のできたコードを提案する
2. レビュイーがレビューコメントを鵜呑みにしてそのまま提出した
レビュイーもコメントの通りに修正をしてしまうのは決して良いことではないです。コメントで提案された内容は必ず手元の環境で検証を行ってから修正をするべきです。
プルリクエストが送信されたコードの実装者はレビュイーです。そのコードの一番の理解者はレビュイーと言えるでしょう。そのため、そのコードには責任を持つべきです。
また、どんなにベテランでどんなに優秀なエンジニアでも、間違ったレビューをすることはあります。なので、信じすぎるのは良くないことです。
ポイントは以下です。
- どんな信頼できるレビュアーからのコメントでも鵜呑みにしない
- 自分で確認・納得してからコードを変更する
- 自分の変更したコードの責任は自分が持つ
Case8: 質問コメントに答えない修正PR
送信したプルリクエストにて、以下のようなコードがプッシュされていたとします。
fruits = ["apple", "banana", "cherry"]
index = 0
fruits.each do |fruit|
puts "#{index}: #{fruit}"
index += 1
end
#=> 0: apple
#=> 1: banana
#=> 2: cherry
このコードに対して、以下のようなやり取りがされていたとします。
質問をされているのに、回答ではなく修正を行なってしまっていますよね。確かに、このコードはeachメソッドを使用するよりもeach_with_indexメソッドを使用した方が簡潔に書けるのですが、each_with_indexメソッド`を使わなければならないという意図ではないはずです。
参考までに、each_with_indexメソッドを使用した場合のコードを載せておきます。
fruits = ["apple", "banana", "cherry"]
fruits.each_with_index do |fruit, index|
puts "#{index}: #{fruit}"
end
今回のケースにおける問題点は以下です。
- コミュニケーションの意図がずれている
- 対話をせずに指摘を即座に受け入れようとしている
1. コミュニケーションの意図がずれている
この問題点は、Case1でもあった問題点と同じです。そちらを参照してください。
2. 対話をせずに指摘を即座に受け入れようとしている
この問題は、経験の浅いエンジニアによく起こりがちなことですね。
レビュイーのコメントに対して改善しようという姿勢はとても良いですが、コードレビューの目的は「指摘された内容の修正」ではなく「より良いコードでマージすること」です。レビュアーからもらったコメントが必ず受け入れるべきものであるかは、検証をして判断をするべきです。コメントのいとがわからなかった場合は、意図を確認してから検証・修正を実施すべきでしょう。
コメントについて検証をした結果、受け入れないという判断をする場合は、その理由を明確にレビュイーにコメントで伝えることも必要なことです。
ポイントは以下です。
- 指摘を受けた際に意図を確認する
- 自分の考えを持って意見を述べる
- 指摘をクリアすることをゴールにしない
Case9: レビューポイントがわかりにくいプルリクエスト
以下のようなプルリクエストが送信されたとします。
実装した内容についてなんとなくはわかりますが、具体的には理解できないですね。
特に2つ目については、どのようにロジックを修正したのかわかりませんよね。
今回のケースにおける問題点は以下です。
- 「何のためにどこを変更したか」がわかりづらい
- レビュアーにチェックしてほしいポイントがわかりづらい
1. 「何のためにどこを変更したか」がわかりづらい
複数の変更を一つのコミットにまとめてしまうと、レビューがしづらくなります。同一のプルリクエストでも変更ごとにコミットを分けることで、レビューがしやすくなります。
Gitのコミット履歴から、過去の実装がどのように行われたのかをすぐ確認できるようにするためにも、変更内容ごとにコミットを分けることはベストプラクティスと言えそうです。
- 後から見ても変更意図と変更内容を結びつけられるように配慮する
2. レビュアーにチェックしてほしいポイントがわかりづらい
プルリクエストの詳細で、変更点に対して何をレビューして欲しいのか記載しておくことで、レビュアーは確認すべき内容を理解することができます。
- レビューして欲しいポイントを具体的に伝える
今回のケースのプルリクエストは、最低限チェックしてほしいポイントを記載していたら、内容を理解しやすくなり、レビュアーが何をしたら良いのかが明確になります。
Case10: 気を遣いすぎたレビューコメント
レビュアーがtypoを見つけたとします。その時にレビュアーから以下のようなコメントがされたとします。
周りくどくて何を伝えたいのかがわかりませんね...
プルリクエストのコメントで、超遠回しにコメントをされてもレビュイーには意図は伝わりません。コメントで傷つけてしまうことを恐れて遠回しに指摘することが逆にレビュイーにストレスを与えてしまうことにもなりうるため、配慮はしつつ、意図が正確に伝わるようにコメントをしましょう。
問題点は以下です。
- レビュアーとレビュイーの間に信頼関係が築けておらず、レビュアーが萎縮している
- レビュアーが気を遣いすぎて、まわりくどいレビューコメントになっている
1. レビュアーとレビュイーの間に信頼関係が築けておらず、レビュアーが萎縮している / 2. レビュアーが気を遣いすぎて、まわりくどいレビューコメントになっている
実際、周りくどくて何を伝えたら良いのかわかりませんでしたよね。このようなコメントには、以下のような欠点があります。
- コメントの意図が読み取りづらい
- コメントを書くのにも答えるのにもコストがかかる
- 関係性によっては嫌味と捉えられかねない
このようなコメントをしてしまう原因として、「間違ったコメントをしても大丈夫」という信頼関係が気づけていないことが挙げられます。
レビュアーもレビュイーも、信頼関係が気づけていないと感じた場合には、一つ一つコメントを見直したり、「遠慮なく率直にコメントしてもらって大丈夫だよ」と伝えることも必要なのではないでしょうか。レビュイーも相手を信頼して、率直な言葉選びをするように心がけましょう。
ポイントは以下です。
- 相手を信頼して、率直な言葉を選ぶ
- 率直な言葉でコミュニケーションして問題ないことを、相手に伝え続ける
まとめ
Case1~10までまとめてみました。内容を抜粋して、自分の解釈しやすい形にまとめているので、詳細については書籍を購入して読んでください。
以上