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.1k
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
Honoアップデート 2025年夏
yusukebe
1
910
機能追加とリーダー業務の類似性
rinchoku
2
1k
AIでLINEスタンプを作ってみた
eycjur
1
220
MCPでVibe Working。そして、結局はContext Eng(略)/ Working with Vibe on MCP And Context Eng
rkaga
5
1.9k
🔨 小さなビルドシステムを作る
momeemt
3
660
Testing Trophyは叫ばない
toms74209200
0
680
Improving my own Ruby thereafter
sisshiki1969
1
150
旅行プランAIエージェント開発の裏側
ippo012
2
850
OSS開発者という働き方
andpad
5
1.7k
モバイルアプリからWebへの横展開を加速した話_Claude_Code_実践術.pdf
kazuyasakamoto
0
310
MCPとデザインシステムに立脚したデザインと実装の融合
yukukotani
4
1.3k
Rancher と Terraform
fufuhu
2
200
Featured
See All Featured
Designing for humans not robots
tammielis
253
25k
A designer walks into a library…
pauljervisheath
207
24k
A Tale of Four Properties
chriscoyier
160
23k
実際に使うSQLの書き方 徹底解説 / pgcon21j-tutorial
soudai
PRO
186
54k
A better future with KSS
kneath
239
17k
Optimising Largest Contentful Paint
csswizardry
37
3.4k
What's in a price? How to price your products and services
michaelherold
246
12k
Keith and Marios Guide to Fast Websites
keithpitt
411
22k
The MySQL Ecosystem @ GitHub 2015
samlambert
251
13k
Raft: Consensus for Rubyists
vanstee
140
7.1k
The World Runs on Bad Software
bkeepers
PRO
70
11k
Bash Introduction
62gerente
615
210k
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/