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