快適なコードレビューを目指して / For a comfortable code review

Code Review Meetup #4 の資料です

Transcript

1. 快適なコードレビューを目指して Code Review Meetup #4 @wata727
2. コードレビューに対する期待 • 問題を防ぐ • スタイルの統一 • より良い実装の模索 • 知識の共有
3. 問題を防ぐ ✔ 障害を引き起こすリスクは無いか？ ✔ 仕様を満たしているか？
4. スタイルの統一 ✔ コーディング規約に倣っているか？
5. より良い実装の模索 ✔ 後から見たときに理解しやすいか？ ✔ 変更に強いか？ ✔ バグが発生したときにすぐ気付けるか？
6. 知識の共有 ✔ プロジェクトの背景や仕様 ✔ 歴史的経緯 ✔ 言語やフレームワークそのもの
7. 大変ですよね？？ すべてをレビューする必要はない、が... 真面目にやるとそれなりに手間も時間もかかる 何とかして負荷を減らせないだろうか？
8. コードレビューに対する期待 • 問題を防ぐ • スタイルの統一 • より良い実装の模索 • 知識の共有 この2つに絞って話をします
9. 問題を防ぐ
10. どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問 題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも 問題があればコメントする
11. どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問 題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも 問題があればコメントする ここをもっと楽にする
12. Phinder: PHP Code Piece Finder • https://github.com/tomokinakamaru/phinder • パターンにマッチするコードがあれば、確認を促 すことができる
13. 簡単な例 In_array 関数の第三引数が省略されていたら確認 を促したい in_array("string", [0, 1]); # => true in_array("string", [0, 1], true); # => false
14. パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき
15. パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき 識別子
16. パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき レビューコメントのきっかけになる コードのパターンを書く 大体PHPの式が書ける ? は任意の式に一致する
17. パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき 何を確認して欲しいのか Gitのコミットメッセージのイ メージで書く
18. パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき どんなときに無視して良 いのか書く
19. 結果 $ phinder ./test.php:3:1 in_array("string", [0, 1]) 第三引数を省略したとき、 型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php (in_array_without_3rd_param)
20. どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問 題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも 問題があればコメントする
21. どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問 題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも 問題があればコメントする Phinderで自動化する レビュイーの仕事にする
22. 例: もっと大雑把に書く - id: db_long_transaction pattern: DB::Transaction message: | トランザクションが長くなる場合にはキューを使いましょう 長いトランザクションを同期的に処理するとレスポンス速度の低下 などの問題を招きます。キューを使うことで非同期に処理ができます justification: トランザクションが十分短いとき
23. 例: もっと大雑把に書く - id: db_long_transaction pattern: DB::Transaction message: | トランザクションが長くなる場合にはキューを使いましょう 長いトランザクションを同期的に処理するとレスポンス速度の低下 などの問題を招きます。キューを使うことで非同期に処理ができます justification: トランザクションが十分短いとき 問題とはほとんど無関係なパ ターンだが、確認を促す起点 としては十分
24. 例: Postmortemと一緒に書く - id: do_not_rename_column pattern: (?)->renameColumn(...) message: | カラムのリネーム時にはサービス停止が起きる可能性があります 過去にカラムをリネームした際にデータベースにアクセスできなくなった 障害が発生しました。同様の問題を起こさないか確認してください。 参考: https://foo.example.com/postmortem/12 justification: トランザクションが十分短いとき
25. 知識の共有
26. 経験をYAMLに書き出す 問題を防ぐときに書いたように、プロジェクト固有の 問題や知識、注意するべきポイントをYAMLファイル に溜め込んでいく プロジェクトの経験や知識を可視化できる
27. 例: APIの注意すべきポイント - id: subscription_is_not_active_when_trial pattern: (?)->isActive(...) message: | SubscriptionクラスのisActiveメソッドはトライアルを含みません isActiveメソッドは歴史的経緯により、トライアルの場合には falseを返します。トライアルを考慮する場合には注意してください justification: トライアルを考慮する必要が無いとき
28. 安心してルールを書くために
29. テストを書きましょう 書いたパターンは本当に正しいですか？ 実は全然見当違いだった... となると勿体無い
30. 例: テストを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき test: fail: in_array("string", [1, 2]) pass: in_array("string", [1, 2], true)
31. 例: テストを書く $ phinder No Error $ phinder `in_array(2, $arr, false)` does not match the rule sample.in_array_without_3rd_param but should match that rule. エラーが無いとき エラーがあるとき
32. 実際の運用
33. ローカルで使う • エディタなどで開発しながら確認する • gitのpre-hookでコミット前に確認する
34. プルリクエストのフックで使う • Jenkinsなどで実行して結果をコメント • reviewdogなどを使う ◦ https://github.com/haya14busa/reviewdog
35. Siderで使う • 近日リリース予定です :tada:
36. Thank you! https://sider.review/