Upgrade to Pro
— share decks privately, control downloads, hide ads and more …
Speaker Deck
Features
Speaker Deck
PRO
Sign in
Sign up for free
Search
Search
快適なコードレビューを目指して / For a comfortable code review
Search
Kazuma Watanabe
September 27, 2018
Programming
1
660
快適なコードレビューを目指して / For a comfortable code review
Code Review Meetup #4 の資料です
Kazuma Watanabe
September 27, 2018
Tweet
Share
More Decks by Kazuma Watanabe
See All by Kazuma Watanabe
SmartHRにおけるBiTemporal Data Modelの実践のその後 / After the practice of BiTemporal Data Model in SmartHR
wata727
1
3.3k
PHPを検査するPHPを書く / Write PHP inspection by PHP
wata727
1
2.3k
現実世界でのコンテナの運び方
wata727
3
1.2k
Lintの付き合い方とPahoutのご紹介
wata727
0
170
Querlyで始めるコードレビューの自動化
wata727
2
450
コンテナをSpot Fleetで起動するという選択肢
wata727
2
1.1k
エンジニア向けSaaSを支えるInfrastructure as Code
wata727
5
2.4k
SideCIのインフラ構築を自動化した話
wata727
1
2.2k
Other Decks in Programming
See All in Programming
monorepo の Go テストをはやくした〜い!~最小の依存解決への道のり~ / faster-testing-of-monorepos
convto
2
560
「ちょっと古いから」って避けてた技術書、今だからこそ読もう
mottyzzz
12
7.2k
CSC305 Lecture 08
javiergs
PRO
0
280
Foundation Modelsを実装日本語学習アプリを作ってみた!
hypebeans
0
130
組込みだけじゃない!TinyGo で始める無料クラウド開発入門
otakakot
2
380
iOSでSVG画像を扱う
kishikawakatsumi
0
170
The Past, Present, and Future of Enterprise Java
ivargrimstad
0
280
CSC305 Lecture 11
javiergs
PRO
0
280
釣り地図SNSにおける有料機能の実装
nokonoko1203
0
200
AI Agent 時代的開發者生存指南
eddie
4
2.1k
Cursorハンズオン実践!
eltociear
2
1.2k
EMこそClaude Codeでコード調査しよう
shibayu36
0
440
Featured
See All Featured
10 Git Anti Patterns You Should be Aware of
lemiorhan
PRO
658
61k
Raft: Consensus for Rubyists
vanstee
140
7.2k
Stop Working from a Prison Cell
hatefulcrawdad
272
21k
Building Adaptive Systems
keathley
44
2.8k
Writing Fast Ruby
sferik
630
62k
Building an army of robots
kneath
305
46k
The Cost Of JavaScript in 2023
addyosmani
55
9.1k
Optimising Largest Contentful Paint
csswizardry
37
3.5k
No one is an island. Learnings from fostering a developers community.
thoeni
21
3.5k
XXLCSS - How to scale CSS and keep your sanity
sugarenia
249
1.3M
Learning to Love Humans: Emotional Interface Design
aarron
274
41k
Art, The Web, and Tiny UX
lynnandtonic
303
21k
Transcript
快適なコードレビューを目指して Code Review Meetup #4 @wata727
コードレビューに対する期待 • 問題を防ぐ • スタイルの統一 • より良い実装の模索 • 知識の共有
問題を防ぐ ✔ 障害を引き起こすリスクは無いか? ✔ 仕様を満たしているか?
スタイルの統一 ✔ コーディング規約に倣っているか?
より良い実装の模索 ✔ 後から見たときに理解しやすいか? ✔ 変更に強いか? ✔ バグが発生したときにすぐ気付けるか?
知識の共有 ✔ プロジェクトの背景や仕様 ✔ 歴史的経緯 ✔ 言語やフレームワークそのもの
大変ですよね?? すべてをレビューする必要はない、が... 真面目にやるとそれなりに手間も時間もかかる 何とかして負荷を減らせないだろうか?
コードレビューに対する期待 • 問題を防ぐ • スタイルの統一 • より良い実装の模索 • 知識の共有 この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]); # => true
in_array("string", [0, 1], true); # => false
パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください
in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき
パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください
in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき 識別子
パターンを書く - 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の式が書ける ? は任意の式に一致する
パターンを書く - 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のコミットメッセージのイ メージで書く
パターンを書く - id: in_array_without_3rd_param pattern: in_array(?, ?) message: | 第三引数を省略したとき、型の比較が行われないので注意してください
in_array関数の第三引数は省略されたとき false です。 このとき、各要素は緩やかな比較によって比較されます。 参考: https://secure.php.net/manual/ja/function.in-array.php justification: 緩やかな比較を期待するとき どんなときに無視して良 いのか書く
結果 $ 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. それらの問題と現状の変更を踏まえて、それでも
問題があればコメントする
どうレビューするか 1. Diffを確認 2. 怪しいパターンが無いか探す 3. 見つかったらパターンに関連しておきた過去の問 題や懸念を探したり、思い出したりする 4. それらの問題と現状の変更を踏まえて、それでも
問題があればコメントする Phinderで自動化する レビュイーの仕事にする
例: もっと大雑把に書く - id: db_long_transaction pattern: DB::Transaction message: | トランザクションが長くなる場合にはキューを使いましょう
長いトランザクションを同期的に処理するとレスポンス速度の低下 などの問題を招きます。キューを使うことで非同期に処理ができます justification: トランザクションが十分短いとき
例: もっと大雑把に書く - id: db_long_transaction pattern: DB::Transaction message: | トランザクションが長くなる場合にはキューを使いましょう
長いトランザクションを同期的に処理するとレスポンス速度の低下 などの問題を招きます。キューを使うことで非同期に処理ができます justification: トランザクションが十分短いとき 問題とはほとんど無関係なパ ターンだが、確認を促す起点 としては十分
例: Postmortemと一緒に書く - id: do_not_rename_column pattern: (?)->renameColumn(...) message: | カラムのリネーム時にはサービス停止が起きる可能性があります
過去にカラムをリネームした際にデータベースにアクセスできなくなった 障害が発生しました。同様の問題を起こさないか確認してください。 参考: https://foo.example.com/postmortem/12 justification: トランザクションが十分短いとき
知識の共有
経験をYAMLに書き出す 問題を防ぐときに書いたように、プロジェクト固有の 問題や知識、注意するべきポイントをYAMLファイル に溜め込んでいく プロジェクトの経験や知識を可視化できる
例: APIの注意すべきポイント - id: subscription_is_not_active_when_trial pattern: (?)->isActive(...) message: | SubscriptionクラスのisActiveメソッドはトライアルを含みません
isActiveメソッドは歴史的経緯により、トライアルの場合には falseを返します。トライアルを考慮する場合には注意してください justification: トライアルを考慮する必要が無いとき
安心してルールを書くために
テストを書きましょう 書いたパターンは本当に正しいですか? 実は全然見当違いだった... となると勿体無い
例: テストを書く - 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)
例: テストを書く $ 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. エラーが無いとき エラーがあるとき
実際の運用
ローカルで使う • エディタなどで開発しながら確認する • gitのpre-hookでコミット前に確認する
プルリクエストのフックで使う • Jenkinsなどで実行して結果をコメント • reviewdogなどを使う ◦ https://github.com/haya14busa/reviewdog
Siderで使う • 近日リリース予定です :tada:
Thank you! https://sider.review/