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
530
快適なコードレビューを目指して / 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
1.8k
PHPを検査するPHPを書く / Write PHP inspection by PHP
wata727
1
1.8k
現実世界でのコンテナの運び方
wata727
3
1k
Lintの付き合い方とPahoutのご紹介
wata727
0
140
Querlyで始めるコードレビューの自動化
wata727
2
410
コンテナをSpot Fleetで起動するという選択肢
wata727
2
1k
エンジニア向けSaaSを支えるInfrastructure as Code
wata727
5
2.1k
SideCIのインフラ構築を自動化した話
wata727
1
2k
Other Decks in Programming
See All in Programming
実践!RDRAを活用した既存システムの仕様変更 / Specification Changes in Existing Systems Utilizing RDRA
imamotohikaru
0
2.6k
The Future of C++ Interoperability: Insights from Porting a Game to Swift
teamhimeh
0
270
CSRF対策のやり方、そろそろアップデートしませんか / Update your knowledge of CSRF protection
hiro_y
25
15k
今の SmartHR にエンジニアで入社するとどうなるの?
daisukeshinoku
3
780
品質が高いコードって何?Rev2.1
ickx
1
490
Go1.22からの疑似乱数生成器について/go-122-pseudo-random-generator
convto
1
160
Learning PHP and Static Analysis with PHP Parser
inouehi
1
250
MySQL のインデックスの種類をおさらいしよう! / overviewing indexes in MySQL
okashoi
0
170
App Router への移行は「改善」となり得るのか?/ Can migration to App Router be an improvement
takefumiyoshii
1
120
WebComponentsで フレームワークを1ページに共存させる
webuilder240
0
150
脱・初心者!脱・マネコン!AWS CDKを使ってみませんか!?
har1101
0
140
TCAの Shared Stateって どういう仕組みになってんの?
yimajo
0
330
Featured
See All Featured
Learning to Love Humans: Emotional Interface Design
aarron
266
39k
The Mythical Team-Month
searls
214
42k
Music & Morning Musume
bryan
39
5.5k
The Power of CSS Pseudo Elements
geoffreycrofte
58
4.9k
Testing 201, or: Great Expectations
jmmastey
27
6.3k
Understanding Cognitive Biases in Performance Measurement
bluesmoon
6
950
Automating Front-end Workflow
addyosmani
1353
200k
A better future with KSS
kneath
230
16k
RailsConf & Balkan Ruby 2019: The Past, Present, and Future of Rails at GitHub
eileencodes
124
32k
Code Review Best Practice
trishagee
54
15k
A Tale of Four Properties
chriscoyier
150
22k
Designing Experiences People Love
moore
135
23k
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/