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.2k
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
CSC509 Lecture 03
javiergs
PRO
0
330
PHPに関数型の魂を宿す〜PHP 8.5 で実現する堅牢なコードとは〜 #phpcon_hiroshima / phpcon-hiroshima-2025
shogogg
1
140
技術的負債の正体を知って向き合う / Facing Technical Debt
irof
0
160
SpecKitでどこまでできる? コストはどれくらい?
leveragestech
0
690
止められない医療アプリ、そっと Swift 6 へ
medley
1
160
Le côté obscur des IA génératives
pascallemerrer
0
140
デミカツ切り抜きで面倒くさいことはPythonにやらせよう
aokswork3
0
230
CSC305 Lecture 04
javiergs
PRO
0
270
Go Conference 2025: Goで体感するMultipath TCP ― Go 1.24 時代の MPTCP Listener を理解する
takehaya
9
1.7k
overlayPreferenceValue で実現する ピュア SwiftUI な AdMob ネイティブ広告
uhucream
0
180
組込みだけじゃない!TinyGo で始める無料クラウド開発入門
otakakot
0
230
CSC509 Lecture 06
javiergs
PRO
0
260
Featured
See All Featured
GitHub's CSS Performance
jonrohan
1032
470k
The Art of Programming - Codeland 2020
erikaheidi
56
14k
Keith and Marios Guide to Fast Websites
keithpitt
411
23k
Become a Pro
speakerdeck
PRO
29
5.5k
Git: the NoSQL Database
bkeepers
PRO
431
66k
A better future with KSS
kneath
239
18k
Side Projects
sachag
455
43k
Practical Orchestrator
shlominoach
190
11k
Save Time (by Creating Custom Rails Generators)
garrettdimon
PRO
32
1.6k
Visualization
eitanlees
149
16k
The Web Performance Landscape in 2024 [PerfNow 2024]
tammyeverts
9
860
Let's Do A Bunch of Simple Stuff to Make Websites Faster
chriscoyier
507
140k
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/