コードレビュー研修

2020/07/21 に弊社新卒向けに実施したコードレビュー研修の資料です。

Transcript

1. コードレビュー研修 Yuki Toyoda @ CyberAgent AI Tech Studio
2. 自己紹介 • Yuki Toyoda • Tech Lead @ AI tech studio • tw: @helloyuki_
3. この研修では • Brand-new 2020 に向けた研修資料です。 • 何を話すべきかよくわからなかったので、レビューに際して 私が気をつけていることを書きました。 ◦ わたしはこういったエモ寄りの話はほとんどしないので、貴重な機会です（？） • 開発プロセスの中にレビューの実施がすでに盛り込まれていることを前提としています。 ◦ なので、そもそもチームにどうレビューを根付かせるかという話は含まれていません。
4. 目次 1. なぜコードレビューをするのか 2. レビューをする側: コードレビューでは何をするのか 3. レビューをされる側: プルリクエストの作り方
5. なぜコードレビューをするのか
6. なぜコードレビューをするのか 1. 単純にプロダクトの品質担保のため。 2. プロダクトやコード上の知識を共有するため。 3. （新人さん向け）他人のコードを読む練習をするため。
7. 単純にプロダクトの品質担保のため • 「品質担保」の定義は難しいが、一般には下記を行うと品質担保がある程度できる。 ◦ 実現したい仕様を正しく実現できているかを見る。 ◦ 事前に不具合を見つける。 ◦ テストコードが書いてない箇所を見つける。 • レビュー中に見るべき点については後ほど解説する。
8. プロダクトやコード上の知識を共有するため • いわゆるメンタリングや歴史の記録も目的。 • 開発にはどうしても「暗黙知」がつきまとう。 • レビューでお互いにコードを見合いながら批評し GitHub 上にコメントしあうことで、暗黙知だった内容を 形式知に変えていくことができる。 • 知識を一部だけに滞留させておかずに、チーム全体に行き渡らせるためにレビューという機会を利用でき る。
9. トピックス: 暗黙知と形式知 • 暗黙知そのものは、もともとはマイケル・ポランニーという思想家がいい出したもの。それを経営学者の野 中郁次郎がより体系的にまとめ直した（ SECI モデル）。 • 暗黙知: 経験的に使用しているが、表現が困難で、記述しにくい知識のこと。 • 形式知: 表現が可能で、記述できる知識のこと。
10. 他人のコードを読む練習をするため • 他人のコードを読むのは意外と難しい。 ◦ なぜなら、その人の思考回路を一度理解する必要があるため。 ◦ 思考回路は人によってかなり差がある。ゆえに難しい。 • 他人のコードを読む機会を強制的に作れるという意味で、コードレビューは重要。 • 実装を真似すると、もっとも低コストで正しい方向への成長を達成できる。
11. トピックス: すべては真似ることから始まる • 「『パクる』は悪いことか？」 →「NO」 • 芸事の世界では「守・破・離」という言葉があり、このうちの守を達成しやすくなる。 • 「学ぶ」の語源は「真似ぶ」と同根で、これは要するに真似することを意味する。 • アリストテレスはアーツの本質をミメーシスという言葉に込めた。ミメーシスは模倣の意。 • …もっとも、エンジニアリングはサイエンスであって欲しいが。
12. レビューをする側 コードレビューでは何をするのか
13. コードレビューでは何をするのか • ざっとチェックポイントを見る。 • コメントを書く。 • レビュを受ける人はそれを直す。 • 再度見る。OK ならここで終了。
14. コードレビューでは何をするのか • ざっとチェックポイントを見る。 • コメントを書く。 • レビュを受ける人はそれを直す。 • 再度見る。OK ならここで終了。
15. チェックポイント（前提） • レビューをし合う。みんなでやる。 ◦ 詳しい人の承認をもらう形式にするとちょっと安心できるけど、基本はみんなでやる。 • すべてをチェックはしきれない。 ◦ レビューしなければまずい箇所を見逃すことはある。 ◦ 不具合の発見はユニットテストなどでも行う。 • レビューが正しく機能するためにはそもそも 仕様がきっちり決まっていないと無理。 ◦ 仕様がないor文章化されておらず共通認識が取れていない状態でレビューをしても、本当に果たしたかったレ ビューの目的は果たせない。 • レビューを通じてディスカッションをする。 ◦ 客観的な視点を取り込みながらコードはよくなっていくというのを忘れない。
16. チェックポイント 下記は私が個人的に見ているところ。 • 明らかに仕様と違う実装がされていないか？ • コードの統一感はとれているか？ ◦ キャメルケース・スネークケース。関数名が適切か。フォーマッタはあたっているか。 ◦ リンターに任せてしまうプロダクトも多いかも。 • ファイルの配置場所、パッケージの構成は合理的か？ ◦ アーキテクチャの意図する場所に置かれているかを見ます。 • パフォーマンス上明らかにまずそうなものはないか？ ◦ N+1 をやっていそう、計算量が明らかに多い、など。
17. チェックポイント 下記は私が個人的に見ているところ。 • 言語仕様上まずそうな実装はされていないか？ ◦ その言語が他の言語とはすこし違う挙動をする箇所を指摘します。 • 並列処理・並行処理周りで意図しない挙動になりそうな箇所はないか？ ◦ データ競合が起きるなど。 • ユニットテスト ◦ はあるか？ ◦ を行ってはいるが無意味なものになっていないか？ ◦ は仕様通りのケースが書かれているか？
18. トピックス: 「可読性」 私は「可読性」というレビューコメントにはほとんど意味がないと思っている。理由は、 • ツールによって担保できる。リンターやフォーマッタがあるならそれに従うべきだから。 • 変数名や関数名は、そもそも設計をチーム全体で行っていれば自ずと決まるから。 • 可読性というのは、その言語に対する経験やミドルウェアの習熟など、外部要因によって決まることがほ とんどだから。 レビューでは、「可読性が低い」という言葉で済ませずに、 より具体的に指摘をする。
19. コードレビューでは何をするのか • ざっとチェックポイントを見る。 • コメントを書く。 • レビューを受ける人はそれを直す。 • 再度見る。OK ならここで終了。
20. コメントを書く際の心構え • コードレビューはコードの欠陥を指摘する行為ではない 。 • したがって、欠陥を指摘しようと意気込まない 。 • レビューは誰もが平等にするし、またレビューを受ける必要がある。 • よりよいソフトウェアにするために 議論する場と考える。 • 自身の主張の正当性を、レビューコメントの辛辣さや皮肉で主張しない 。我々エンジニアは科学的手法と 理性による判断の正しさを信条として生きるべき職種である。あくまでロジカルに。
21. • よいコメントとは、納得感のあるコメント。 • 納得感を生むために ◦ 衒学的なコメントをせず、より具体的なコメントをする。 ◦ 主張の理由を必ず示す。 ◦ 主張の客観性を担保する。 よいコメントの書き方
22. • 「ここはオブジェクト指向ではありませんね」「関数型らしく書いてください」「 DDD によると XXX (XXX は DDD の用語) ではない」のような、ある種の専門用語をふりかざす だけのコメントをしない。 • 「データの内容は、一般的にはひとつクラスや構造体を用意して、その中で扱うようにします。こうすると、 バラバラだったデータの内容に対して我々が普段業務で使う言葉で命名することができ、より可読性が上 がり、プログラマ間での意思統一がはかりやすくなると思います。具体的には、下記のコードのように直し ます。」など。 • 具体的にどうしてほしくて、結果何が嬉しいのかを詳細に書く。 コードを交える。 • どうしても固有の用語を使用したい場合は、レビューを受ける人の理解度に応じて参考リンクを貼るなど する。 衒学的なコメントをせず、より具体的に
23. • 「この関数の引数にはこれを入れるべきではありません」といった主張をする場合には、必ず論拠を示 す。 ◦ 「なぜなら、関数の引数にはその関数に必要な情報のみが入っているべきで、コンテキストのような環境情報が 入っているべきではないからです」など。 • PREP 法を意識する。結論→論拠→例示→結論の順に文章を書く方法。人に物事を伝える際に使えるフ レームワーク。 主張の理由を必ず示す
24. • 主張には客観性がなければならない。 • 客観性の担保には、下記が使える。 ◦ ネットの記事で構わないので、参考情報を付与する。 ◦ 公式サイトやドキュメントからの引用をする。 ◦ 先行事例や参考情報、手法（プロダクト内のものでも可）が他にあるのであれば出す。 ◦ 他にもあるかな？ • 自身の主張について第三者も意見して 反駁できる状態にしておく のが理想。そのために論拠やソースが 必要。論拠やソースを辿って思考手順が正しいかを見たいから。これを反証可能性と呼ぶが、科学では もっとも重要な営みの一つ。 主張の客観性を担保する
25. オススメ（学生時代に読んだかも？） • 『理科系の作文技術』 • とくに新人のうちは「事実と意見」の章を重点的に読んでください。 • 本当はきちんとロジカルシンキングを教えたほうがよいのかも？
26. • 言い回しに気をつける。 ◦ 「〜です。直してください。」（命令形）より、「〜は、〜がよいと思いました。いかがでしょう？」提案の形を取るとよ い。疑問形で終わらせると何かしら意見を述べる必要が出てくるので、ディスカッションも起こせて一石二鳥。 ◦ 体調不良時、あるいは余裕がない時にコメントを書くと相手をムッとさせることがあるので、一度落ち着いてから 十分に時間を確保して書く。 ←一番大事 ◦ もちろん辛辣なコメントや皮肉は論外。 ◦ レビューを受ける人も人間であることを忘れない。 • 人に反感を抱かせたり、不快にさせたりしていいことはあまりない。 • 辛辣さや皮肉は癖になるので、日頃からそれに頼らないように気をつける。 人への配慮
27. 実際に行われているやりとり • コメント内で具体性の掘り下げが要求されている。 • 具体的なコードや手段でもって議論がなされている。 社内にのみ公開しました
28. 実際に行われているやりとり • 提案や確認の形をとっている。 • 内部ソースを提示して客観性をもたせている。 社内にのみ公開しました
29. 実際に行われているやりとり • そもそも先にデータモデルの設計はやっておくべきという話は ありますが… • プロダクト全体としてどうしているかを書いている。 • 集約という概念をチームに伝えておきたかったので、その話を している。 社内にのみ公開しました
30. 実際に行われているやりとり • つづき 社内にのみ公開しました
31. 実際に行われているやりとり • 先ほどしたコメントに対する反駁（というより提案） 社内にのみ公開しました
32. 実際に行われているやりとり • ディスカッションの結果 • 新たな概念を導入する際には参考ソースを示す。読んでもらっ て納得してもらってから実装に入ってもらう。 • 何か問題点があった場合にはそれも書いておく。 • 実装そのものは相手に委ね、細かく指示しない。 ◦ ただし、相手の実力によって使い分ける。 社内にのみ公開しました
33. レビューをされる側 プルリクエストの作り方
34. よいプルリクエストを作るには 私がレビューしやすいなと感じる PR に共通するポイントは下記。 • PR の概要が書かれている。 • 差分（あるいは PR ）の目的が1つに定まっている。 • 差分を大きくしすぎない。かといって細かくすればいいというものでもない。 • 重点的に見てほしいポイントが書いてある。
35. PR の概要が書かれている 当たり前かと思うかも知れないが、必要十分な概要になっていないことは多い。 下記を書いてもらえると、レビューしやすい。 • PR の背景や目的を書く。 ◦ 多くの場合、解決したい問題、追加したい機能が書かれるはず。 • 検証内容を書き、その結果を貼る。 ◦ テストを手動で実施した場合はその内容。 ◦ ユニットテストの概要。 ◦ 画面変更の場合はスクショだとわかりやすい。 • 次のタスクとして何が想定されるかを書く。 ◦ そのコードの評価は他の機能との関係性によって決まるので。
36. 差分（PR）の目的が一つに定まっている • 「X の機能追加」と「Y の不具合修正」を混ぜないようにする。 • やりがちなのは、機能追加中にやりたくなったリファクタリングを混ぜてしまうこと。 • 我慢して別の PR にわけて出すのが吉。最悪の際、ロールバックもしやすい。
37. 差分を適切なサイズに保つ • これは難しいが、よく「一つの PRのdiffを小さくしよう」というベストプラクティスのようなものが言われる。 • かといって、関数ひとつだけ実装して、とくに動きもしないみたいな PR はしんどい。 • 「ひとまとまりに」なっていたほうが、むしろ嬉しい。 • ただ、その「ひとまとまり」が大きくなりすぎると辛い。見逃す。 • 大きくなった場合には理由を書く。自動生成などは大きくなりがちで仕方ない場合もある。 • 差分が大きくなりすぎる場合は、だいたいの場合はタスクの切り方に問題がある。チームでタスクの切り 方を見直す必要がある。
38. 重点的に見てほしいポイントが書いてある • 実装している最中に不安に思った場所が必ずあるはず。 • そういうところは、先にコメントを書いておいてもらえると嬉しい。 • また、もっといい方法がありそうだが思いつかなかった、などのコメントも嬉しい。 • レビュアーを上手に使って、自身の成長の糧にする。
39. 実際に行われているやりとり • diff の概要がリンク付きで書かれている。 社内にのみ公開しました
40. 実際に行われているやりとり • 行った作業の概要が書いてある。 • 検証結果が概要に書いてある。 社内にのみ公開しました
41. 実際に行われているやりとり • diff が多い場合にはその旨と内容が書いてある。 • 重点的にチェックしてほしいことが書いてある。 • やってみたいけど、手法がわからないことが書いてある。 • アドバイスしやすい。 社内にのみ公開しました
42. 質問タイム
43. 参考文献 • アリストテレース『詩学』（岩波文庫） • 『日本文化の核心』（講談社新書）松岡正剛 • SECI モデル: https://mba.globis.ac.jp/about_mba/glossary/detail-11667.html • 反証可能性: https://www.cscd.osaka-u.ac.jp/user/rosaldo/080428falsifiability.html • PREP 法: https://webtan.impress.co.jp/e/2017/06/08/25694 • Code Health: Respectful Reviews == Useful Reviews: https://testing.googleblog.com/2019/11/code-health-respectful-reviews-useful.html • Why review code?: https://sophiebits.com/2018/12/25/why-review-code.html