仕事でのコードレビューについて思っていること / My thoughts about code review at work

仕事でのコードレビューについて思っていることを社内勉強会で話したスライド

スライドについての補足: https://blog.yujigraffiti.com/2020/11/blog-post.html

Transcript

1. 仕事でのコードレビューについて思っていること @yuji_developer 2020/10/23
2. ⾃⼰紹介 Yuji Hanamura Ruby/Rails programmer https://github.com/yujideveloper 2
3. 免責事項＆前提 専⾨に勉強したり研究したりした成果ではない ⽇々のコードレビューを通して思っていること 効果を保証するものでもない 強い⾔葉があっても何かを強制する意図や否定する意図はない（たぶん） あくまでお気持ち お察しください 主にPull Request（Merge Request）ベースのコードレビューの話 いまどきPull Request使っていない場合は…… 3
4. コードレビューしていますか︖ 4
5. 結論 コードレビューはやるやらないを議論するものではない やるのは当たり前 個⼈の感想 どうやるかが問題 5
6. なぜコードレビューするのか コードの事前検査 仕様に即しているか ⼿戻りを減らす コード品質の向上 メンテナンスしにくいコードになっていないかなど コードの共同所有 ⾃分（レビューイー）しか知らない状態を緩和する 知⾒の共有 コードに対する議論 コードレビューはコミュニケーションの場 6
7. コードレビューの対象のコード 本番環境に関係する全てのコード アプリケーションコード テストコード 設定 インフラコード データ修正⽤コード 開発ツールはやらなくても良いがやった⽅が良い 知⾒の共有 特定の誰かしかメンテナンスできない状態は健全ではない データ調査⽤コードはやるのがベター そのコードを実⾏して負荷など⼤丈夫︖ 7
8. コードレビューのコストについて 実装の時間 ＝ コードを書く時間 ＋ コードレビューの時間 コードを書く時間のおおよそ25〜50%くらい 多分これでもギリギリ コードレビューしなければコストを圧縮できると思うのは気のせい テストでバグが出やすくなる メンテナンスしにくいコードが紛れ込む etc. 8
9. コードレビューしなくていい場合を考えてみた ⾃分⽤のコード バグや負債を絶対に⽣み出さない⼈だけで構成されたチーム ニュータイプだけで構成されて以⼼伝⼼出来てしまうチーム 9
10. 誰がレビューアーになるか おすすめはチームのエンジニア全員 〜8⼈くらいのチームを想定 全員が承認しなければマージしないという意味ではない 最低1⼈、2⼈推奨 対象に詳しい⼈ + 他の誰か ランダムアサインとかやっているところもあるみたい 特定のメンバーに固定するのは避けたい コードレビューおじさん問題 10
11. 約束事 コードレビューに社内の⼈事上の上下関係を持ち込まない コードの前に⼈は平等 役職が上の⼈の⾔うことが偉いとか正しいとかはない 役職ではなく役割的な発⾔⼒の差はあるかもしれない ⼈格攻撃をしない 強い⾔葉は控える 意図を相⼿に伝えることに⼿を抜かない 関係者全員が前向きに取り組むことが⼤切 他⼈事ではない レビューイー対レビューアーのような対⽴構図を作らない コードレビューはバトルではない 全員でコードを良くしていこう 11
12. レビューイーの⼼得 1. レビューアーにレビュー対象の説明をする、理解してもらう努⼒をする 2. レビューアーのコメントへのリアクションはなるべく早く 3. レビューアーのコメントを鵜呑みにして修正しない 4. コードレビューを依頼して満⾜しない 12
13. レビューイーの⼼得#1 レビューアーにレビュー対象の説明をする、理解してもらう努⼒をする タイトル・説明欄をちゃんと書く ドキュメントへのリンクなどコードレビューに必要な材料を提⽰する やったこと（事実）だけでなくやった理由や背景なども説明する 選択しなかった他の⽅法がある場合は選択しなかった理由なども説明する ⾒て察してという態度はNG 開始時点でコードレビュー対象に⼀番詳しいのは（恐らく）レビューイー 13
14. レビューイーの⼼得#2 レビューアーのコメントへのリアクションはなるべく早く レビューアーのコメントへの応答 コードの修正 14
15. レビューイーの⼼得#3 レビューアーのコメントを鵜呑みにして修正しない 納得がいくまで議論をする 指摘されたから直しましたみたいな⾏動はNG ⾔われたことの意味が分からなければ調べる、聞く 15
16. レビューイーの⼼得#4 コードレビュー依頼して満⾜しない セルフレビューをする ⾃分でもおかしなところがないか改めて⾒る 作りっぱなしにしない レビューアーの反応がない時はリマインドする 依頼して終わりではない 16
17. レビューイーの⼼得 1. レビューアーにレビュー対象の説明をする、理解してもらう努⼒をする 2. レビューアーのコメントへのリアクションはなるべく早く 3. レビューアーのコメントを鵜呑みにして修正しない 4. コードレビューを依頼して満⾜しない 17
18. レビューアーの⼼得 1. コードレビュー依頼が出たら出来るだけ早く⾒る 2. コードレビューしたら必ずリアクションをする 3. コードレビュー対象に対するコメントをする 4. コードレビュー対象でわからないことは質問する 18
19. レビューアーの⼼得#1 コードレビュー依頼が出たら出来るだけ早く⾒る レビューイーのコードレビュー対象の記憶が鮮明なうちにコメントする 依頼されて1週間後にコメントして、その返事がまた1週間後に来て…… 時間が経てばレビューイーも何やっていたか忘れる テンポは⼤事 19
20. レビューアーの⼼得#2 コードレビューしたら必ずリアクションをする 眺めて終わりにしない コメント、承認、etc. 20
21. レビューアーの⼼得#3 コードレビュー対象に対するコメントをする 対象と関係ない話を持ち出さない ⾼圧的にならない 21
22. レビューアーの⼼得#4 コードレビュー対象でわからないことは質問する 開始時点でコードレビュー対象に⼀番詳しいのは（恐らく）レビューイー 実装の意図などわからないことは積極的に質問していく 動いていそうだからいいやにしない わからないこと、知らないこと、は恥ずかしいことではない それを隠して放置する⽅が良くない 22
23. レビューアーの⼼得 1. コードレビュー依頼が出たら出来るだけ早く⾒る 2. コードレビューしたら必ずリアクションをする 3. コードレビュー対象に対するコメントをする 4. コードレビュー対象でわからないことは質問する 23
24. コードレビューが滞るみたいな話 コードレビューの優先順位が低く設定されている︖ コードレビューの優先順位は⾼い 障害対応などコードレビューより優先すべきものは確かにある 少なくとも通常の⾃分の開発タスクよりは⾼い Pull Requestの単位が⼤きすぎる︖ 少なくとも1⽇1回くらいの頻度 個⼈的にはこれでも⼤きすぎると感じる 全部完成していなくて良い フィーチャートグルなど途中のものを⼊れる⽅法はある 機能追加とリファクタリングを混ぜない 複数の機能追加を混ぜない 24
25. コードレビューについてチーム内で認識合わせするのが良い どういうところを⾒るか どういう⽬的でやるのか 25
26. コードレビューで何をコメントしているか 要件や仕様に関するもの ネーミングに関するもの クラスや関数の構成に関するもの テストコードは⼗分か 提案 質問 26
27. コードレビューで何をコメントしているか#1 要件や仕様に関するもの 要件や仕様を満たしていない そもそも要件や仕様がおかしそう 27
28. コードレビューで何をコメントしているか#2 ネーミングに関するもの クラス名、関数名、変数名、ファイル名、etc. 意味が曖昧 意図が間違って伝わる 品詞がおかしい 命名規則に則っていない 28
29. コードレビューで何をコメントしているか#3 クラスや関数の構成に関するもの なんでもやっちゃうスーパーなクラス・関数 既存のこれが使える 既存の実装に囚われずこうした⽅が良い 29
30. コードレビューで何をコメントしているか#4 テストコードは⼗分か テストケースが⾜りない テストケースが余分 30
31. コードレビューで何をコメントしているか#5 提案 こういう実装の⽅が良いと思う ここはリファクタリングした⽅が良さそう 31
32. コードレビューで何をコメントしているか#6 質問 この実装の意図はなに︖ 扱うデータの想定件数は︖ パフォーマンス⾯の計測した︖ 32
33. コードレビューが全てを解決するわけではない バグがなくなるからテストしなくていいって話にはならない コードレビューで全てのバグを取り切るのは不可能 バグをつぶすのはテストの領域 コードレビューしたからといってレビューアーが全てを理解できるわけではない 承認したから全部わかっているよねと⾔われても困る 33
34. コードレビュー以前にやることはある 前提知識や仕様などの共有 仕様の読み合わせ 勉強会 モブプロ・ペアプロ コメントで延々と会話するよりモブプロ・ペアプロしてしまうのが良いこと もある 決まったことや話題になったことはPull Requestやコミットログに残すと良い コードフォーマッター・Linter 機械的に検知できるフォーマットやコードの怪しさ等は機械に任せたい フォーマットについてコードレビューで延々コメントする／されるのは⾟い とはいえ新⼈などにコメントするのはあり こういうことが気になる⼈になってほしい RuboCop, ESLint, etc. 34
35. 結論 コードレビューはやるやらないを議論するものではない やるのは当たり前 個⼈の感想 どうやるかが問題 35
36. おわり ご清聴ありがとうございました