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
650
快適なコードレビューを目指して / 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
2.9k
PHPを検査するPHPを書く / Write PHP inspection by PHP
wata727
1
2.2k
現実世界でのコンテナの運び方
wata727
3
1.2k
Lintの付き合い方とPahoutのご紹介
wata727
0
160
Querlyで始めるコードレビューの自動化
wata727
2
450
コンテナをSpot Fleetで起動するという選択肢
wata727
2
1.1k
エンジニア向けSaaSを支えるInfrastructure as Code
wata727
5
2.4k
SideCIのインフラ構築を自動化した話
wata727
1
2.1k
Other Decks in Programming
See All in Programming
Deep Dive into ~/.claude/projects
hiragram
14
2.6k
Is Xcode slowly dying out in 2025?
uetyo
1
270
生成AI時代のコンポーネントライブラリの作り方
touyou
1
220
『自分のデータだけ見せたい!』を叶える──Laravel × Casbin で複雑権限をスッキリ解きほぐす 25 分
akitotsukahara
2
640
Agentic Coding: The Future of Software Development with Agents
mitsuhiko
0
100
20250704_教育事業におけるアジャイルなデータ基盤構築
hanon52_
5
790
VS Code Update for GitHub Copilot
74th
2
650
技術同人誌をMCP Serverにしてみた
74th
1
650
初学者でも今すぐできる、Claude Codeの生産性を10倍上げるTips
s4yuba
16
11k
Discover Metal 4
rei315
2
140
Code as Context 〜 1にコードで 2にリンタ 34がなくて 5にルール? 〜
yodakeisuke
0
130
脱Riverpod?fqueryで考える、TanStack Queryライクなアーキテクチャの可能性
ostk0069
0
150
Featured
See All Featured
Keith and Marios Guide to Fast Websites
keithpitt
411
22k
Docker and Python
trallard
44
3.5k
Building Applications with DynamoDB
mza
95
6.5k
A Tale of Four Properties
chriscoyier
160
23k
Imperfection Machines: The Place of Print at Facebook
scottboms
267
13k
Writing Fast Ruby
sferik
628
62k
Balancing Empowerment & Direction
lara
1
430
[RailsConf 2023 Opening Keynote] The Magic of Rails
eileencodes
29
9.6k
Adopting Sorbet at Scale
ufuk
77
9.5k
The World Runs on Bad Software
bkeepers
PRO
69
11k
個人開発の失敗を避けるイケてる考え方 / tips for indie hackers
panda_program
107
19k
The Illustrated Children's Guide to Kubernetes
chrisshort
48
50k
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/