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

by Kazuma Watanabe

Slide 1

Slide 1 text

快適なコードレビューを目指して Code Review Meetup #4 @wata727

Slide 2

Slide 2 text

コードレビューに対する期待 ● 問題を防ぐ ● スタイルの統一 ● より良い実装の模索 ● 知識の共有

Slide 3

Slide 3 text

問題を防ぐ ✔ 障害を引き起こすリスクは無いか？ ✔ 仕様を満たしているか？

Slide 4

Slide 4 text

スタイルの統一 ✔ コーディング規約に倣っているか？

Slide 5

Slide 5 text

より良い実装の模索 ✔ 後から見たときに理解しやすいか？ ✔ 変更に強いか？ ✔ バグが発生したときにすぐ気付けるか？

Slide 6

Slide 6 text

知識の共有 ✔ プロジェクトの背景や仕様 ✔ 歴史的経緯 ✔ 言語やフレームワークそのもの

Slide 7

Slide 7 text

大変ですよね？？すべてをレビューする必要はない、が... 真面目にやるとそれなりに手間も時間もかかる何とかして負荷を減らせないだろうか？

Slide 8

Slide 8 text

コードレビューに対する期待 ● 問題を防ぐ ● スタイルの統一 ● より良い実装の模索 ● 知識の共有この2つに絞って話をします

Slide 9

Slide 9 text

問題を防ぐ

Slide 10

Slide 10 text

どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも問題があればコメントする

Slide 11

Slide 11 text

どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも問題があればコメントするここをもっと楽にする

Slide 12

Slide 12 text

Phinder: PHP Code Piece Finder ● https://github.com/tomokinakamaru/phinder ● パターンにマッチするコードがあれば、確認を促すことができる

Slide 13

Slide 13 text

簡単な例 In_array 関数の第三引数が省略されていたら確認を促したい in_array("string", [0, 1]); # => true in_array("string", [0, 1], true); # => false

Slide 14

Slide 14 text

パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください in_array関数の第三引数は省略されたとき false です。このとき、各要素は緩やかな比較によって比較されます。参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき

Slide 15

Slide 15 text

Slide 16

Slide 16 text

Slide 17

Slide 17 text

Slide 18

Slide 18 text

Slide 19

Slide 19 text

結果 $ 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)

Slide 20

Slide 20 text

Slide 21

Slide 21 text

どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも問題があればコメントする Phinderで自動化するレビュイーの仕事にする

Slide 22

Slide 22 text

例: もっと大雑把に書く - id: db_long_transaction pattern: DB::Transaction message: | トランザクションが長くなる場合にはキューを使いましょう長いトランザクションを同期的に処理するとレスポンス速度の低下などの問題を招きます。キューを使うことで非同期に処理ができます justification: トランザクションが十分短いとき

Slide 23

Slide 23 text

Slide 24

Slide 24 text

例: Postmortemと一緒に書く - id: do_not_rename_column pattern: (?)->renameColumn(...) message: | カラムのリネーム時にはサービス停止が起きる可能性があります過去にカラムをリネームした際にデータベースにアクセスできなくなった障害が発生しました。同様の問題を起こさないか確認してください。参考: https://foo.example.com/postmortem/12 justification: トランザクションが十分短いとき

Slide 25

Slide 25 text

知識の共有

Slide 26

Slide 26 text

経験をYAMLに書き出す問題を防ぐときに書いたように、プロジェクト固有の問題や知識、注意するべきポイントをYAMLファイルに溜め込んでいくプロジェクトの経験や知識を可視化できる

Slide 27

Slide 27 text

例: APIの注意すべきポイント - id: subscription_is_not_active_when_trial pattern: (?)->isActive(...) message: | SubscriptionクラスのisActiveメソッドはトライアルを含みません isActiveメソッドは歴史的経緯により、トライアルの場合には falseを返します。トライアルを考慮する場合には注意してください justification: トライアルを考慮する必要が無いとき

Slide 28

Slide 28 text

安心してルールを書くために

Slide 29

Slide 29 text

テストを書きましょう書いたパターンは本当に正しいですか？実は全然見当違いだった... となると勿体無い

Slide 30

Slide 30 text

例: テストを書く - 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)

Slide 31

Slide 31 text

例: テストを書く $ 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. エラーが無いときエラーがあるとき

Slide 32

Slide 32 text

実際の運用

Slide 33

Slide 33 text

ローカルで使う ● エディタなどで開発しながら確認する ● gitのpre-hookでコミット前に確認する

Slide 34

Slide 34 text

プルリクエストのフックで使う ● Jenkinsなどで実行して結果をコメント ● reviewdogなどを使う ○ https://github.com/haya14busa/reviewdog