Post on 07-Feb-2017
「速」を落とさないコードレビュー
2017-01-28 Forkwell Meetup #3
Productivity Engineering
大仲能史 a.k.a. @onk
自己紹介
• 大仲能史 a.k.a. @onk
• 株式会社ドリコム 11年目
–アプリケーションスペシャリスト
– Rails歴が一番長くて 8 年ぐらい
• 趣味
–コードを読むこと
• 社内リポジトリ 1800 のうち 300 ぐらい watch 中
–社内ツール作成
1
宣伝
• ドリコムは Elixir CONF Japan 2017のGold Sponsorです
–セッションもあるよ
• Elixir, Ruby エンジニアに限らず色んな職種を積極採用中なので交流タイムに声かけてください！
2
今日の話
ユーザに価値を届ける
3
最低限の品質を保つ
レビューの基盤を作る
前提
• 事業会社の話
–人月単価ではないサービスを提供している
• Pull Request駆動開発をしている
–これができていない場合は「マジカルsvnとキュアgit」を見てください
4
http://www.slideshare.net/takafumionaka/20130322-svngit
やってしまったレビュー風景
5
やってしまったレビュー風景
6
100コメントにかかる時間
• Pull Request が出る
• 気づく (5min)
• やりたいことを把握する (5min)
• コメントする (10min)
• レスがある or 修正のコミットがある (120min)
• ↑をn往復 (10～20hour)
• 何も成果がリリースされないまま1日が終わる
7
何もリリースできない徒労感かつ
ひたすら指摘され続けて疲弊
8
このコードレビューでは何をやっていたのか
9
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
10
• コードが仕様を満たしているか
• 仕様に考慮漏れが無いか
• エンバグは無いか
• 良い設計をしているか
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
11
• スペースとかインデントとか typoとか
= の右に半角スペースが2つあります
シングルクォートじゃなくダブルクォートを使うようにしてください
regist m9(^Д^)ﾌﾟｷﾞｬｰ
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
12
• メソッドの長さとかネストの深さとか
http://postd.cc/code-review-without-your-glasses/
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
13
• メソッド/変数名とか簡潔なロジックとか
guard 句で早めに raise したい
Array#detect 使うともっとシンプル
score って変数に kind の値が入ってるけど score じゃないような
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
14
ユーザ入力をそのまま ORDER BY に入れたらダメです。asc, desc かど
うかを確かめてから
HTML を自前で組み立てていて、XSS 脆弱性があります
validate するようにしてください
UNIQUE 制約忘れずにー
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
15
N+1 気を付けてー
不要なインスタンスを作らない
二重ループになっているけど、工夫したらループ1回で済みます
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
16
• コードが仕様を満たしているか
• 仕様に考慮漏れが無いか
• エンバグは無いか
• 良い設計をしているか
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
17
最低限の守りたい品質
コードレビューでやったこと
リリースしたら価値が届く、本来レビューすべき
だったもの
18
• コードが仕様を満たしているか
• 仕様に考慮漏れが無いか
• エンバグは無いか
• 良い設計をしているか
コードレビューでやったこと
• コードフォーマット
• 眼鏡を外してレビュー
• 記述が分かりやすいか
• セキュリティ担保
• パフォーマンス担保
19
最低限を満たしていないと、違和感が酷くて本来のレビューに入ることができない
「最低限の水準を保とう」という話をし過ぎるとスピードが低下する
20
でも最低限の水準にはなる
21
最低限を満たすのにレビューは要るのか
• コードフォーマット
–コードフォーマッタで自動修正
–スタイルガイドを作って配布
• 眼鏡を外したレビュー
–行数やネスト、循環的複雑度やABC Metric等、機械的に指摘できる
• Rubyらしさとか、他のメソッド名との整合性とか
–まだレビューが必要だが、数年後には自動化できそう
22
最低限を満たすのにレビューは要るのか
• セキュリティ担保
–まだレビューが必要だが、良いフレームワークを使うことで足元を撃ち抜かない書き方を強制できる
• パフォーマンス担保
–まだレビューが必要だが、例えば N+1を自動検出するツールは存在するので上手に使おう
23
最低限の品質を機械的にチェックすることができる
24
機械相手に試行錯誤する環境を作るとレビューコストが大幅減
25
本来レビューすべきだったもの
リリースしたら価値が届く、本来レビューすべき
だったもの
26
• コードが仕様を満たしているか
• 仕様に考慮漏れが無いか
• エンバグは無いか
• 良い設計をしているか
本来レビューすべきだったもの
もっと短縮できないだろうか？
27
コード差分の理由を書く
28
http://techlife.cookpad.com/entry/2015/03/30/174713
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
29
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
30
• この PRをマージすると以下のことができるようになりまぁす！
• 仕様書や Issueがあればリンクを貼る
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
31
• 何を考えてこの PRを作ったのか、みたいなのがあれば
• 考えた結果、除外したものがあれば書いて欲しい
• 例) 「GitLabに似たような画面があったので同じmodel 構造にした」「1万件程度なので LIKE検索で十分実用に耐える速度が出せると判断」
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
32
• 方針に沿って実装していく中でレビュアーに伝えるべきものがあれば補足
• コードだけだと実装意図を読み解きづらい場合に図や疑似コードで流れを説明するとか
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
33
• マージボタンを押してもらうための安心できる材料
• 例)「実機で一覧表示を確認した」「DBAにクエリを見てもらってOK
貰ってます」
僕がよく使うフォーマット
• 目的
• 方針
• 実装
• テスト
• 相談
34
• 書いてみたものの自信が無いところとか
• diff の中にラインコメントの形で書いても良い
PRの目的とコード差分が分かるように
もっと短縮できないだろうか？
35
レビューの前提条件を提示する
• レビュアーに求めているレビュー内容を書いておく
–例)リリース直前なので、ブロッカーになるような不具合が無いかどうかのチェックをお願いします
• これによって減るレビュー
– typo の指摘
–そもそも論
36
不要なレビューが減った
もっと短縮できないだろうか？
37
成果物が見える状態でレビューする
• Before/After のスクリーンショットを貼る
–変化する対象がより分かりやすくなるのでマージしやすくなる
• Pull Request単位で自動デプロイする
–動作確認がサクッと終わるのでマージしやすくなる
38
マージまでが速くなった
もっと短縮できないだろうか？
39
方針の段階でレビューする
• 生煮え Pull Request (WIP)
–「migration と routes 書いたら push してね」
–全体を書く前にレビューすることができるので無駄になるコード量が激減する
40
https://speakerdeck.com/ken_c_lo/pull-request-4-designers-githubwoshi-tutapuroguramatodezainafalseitereteibunakai-fa-huro
方針の段階でレビューする
• 設計レビュー
–設計のみを Markdown で書いて Pull Request する
– description に書く場合と違い
• ラインコメントができる
• コメントに対する修正がコミットになり、追いやすい
41
http://blog.shibayu36.org/entry/2016/08/05/103000
もっと短縮できないだろうか？
42
サービス開発はゴールが分からないのが前提
• 不確実な状況下にある
–ユーザが本当に欲しいものって分かってる？
• 今見えるゴールが正解である保証も無い
• リリース後が 8 割
• サービスのリリースはスタートにすぎない
43
速い馬！
リリースを躊躇しない
• PragmaticであることDogmaticにならないこと
• リリースしてからBe Professional Day で綺麗にする
44
https://speakerdeck.com/hirak/mercariday2017-api
ここまでのまとめ
45
レビューの基盤を作る
ユーザに価値を届ける
最低限の品質を保つ
最低限の品質を保つ
• コーディング規約を用意する
• 機械に指摘させることで
–一人で試行錯誤できる状況を作る
46
ユーザに価値を届ける
• レビューは「改善する」行為
–改善は損失を減らすが、何かを作ってはいない
• レビューによってリリースが遅くなるのは本末転倒
• 素早くマージできる状態を作っていこう
–レビュアーとの意思疎通に気を払ってPull Requestを出す
47
レビューの基盤を作る
48
レビューの目的はさまざま
• 最低限の品質担保
• 不安の解消
• 属人化を防ぐ
• レビュイーの教育
• チームビルド
• etc…
49
関係性のアンチパターンがある
50
http://www.songmu.jp/riji/entry/2014-01-19-cross2014.html
レビュアー・レビュイーの関係性
• 問題 vs. 私たち、という大前提
–レビュイーは人格攻撃と受け取らない
–レビュアーは権威的になってはいけない
• フィードバックは感情を押し付けるものではなく、受け手が成果物をより良くするために必要な情報を届けること
51
心理的安全性
• 不具合のある状態でリリースしたい人は居ない
–レビュイーは悪意でバグを作っているわけではない
–レビュアーは悪意で指摘しているわけではない
• というのを分かりやすくするために
–レビュイーは前提をしっかり提示する
–レビュアーは伝え方を丁寧にする
52
レビューは取り込むもの
• 本来コードレビュー文化のあるべき正しき姿はチーム間で議論をしてサービスにとって最終的に良いと判断したことを取り込むこと
• レビュアーからレビュイーへの一方的な指摘にしない
–納得をして「取り込む」
–納得していれば「指摘対応」というコミットメッセージにはならない
53
http://yutokyokutyo.hatenablog.com/entry/20161213/1481590322
レビューは取り込むもの
• 納得していなかったら同じ指摘を繰り返すことになる
• レビュイーは納得していない指摘を繰り返し受ける
–やらされる感++
–自己効力感—
• 権威に頼りつつレビューする
–「条件式が分かりづらいです。リーダブルコード 7.1 参照」
–リーダブルコード、リファクタリングがド安定
54
レビュアーが固定されている
• 常にベテラン→ ルーキーの方向のレビューしか無い状態は健全ではない
–ベテランの無駄遣い問題
• レビューコストを外して開発に専念して欲しい
–ベテランを信用しすぎ問題
–ルーキーの成長が悪い問題
• レビューはする側に回った方が教育効果が大きい
55
レビュイーからレビュアーへ
• 二段階レビューを導入
–ルーキーがレビューして、その後ベテランがレビューする
–ルーキーが見落としするたびに心に爪痕が残り、学ぶ
–ルーキーで役に立つのか？は、やり方次第
• 「ここよく分かんないです」を言うだけで価値がある
• 分かりづらい部分はだいたい怪しい
• テスト書いてなくね？も言って欲しい
• ルーキーから指摘する障壁をどんどん取り除きたい
56
レビュイーからレビュアーへ
• レビュータイムを設ける
–全員がレビュアーとなる
–ルーキーからレビューする障壁を外す
–溜まったレビュー待ちの Pull Requestが消化される良い副作用もある
• 最初はそちらを目的にしていた
57
まとめ
58
今日の話
ユーザに価値を届ける
59
最低限の品質を保つ
レビューの基盤を作る
レビューの基盤を作る
• レビュイーとレビュアーの関係性に気を遣う
–「問題 vs. 私たち」
–「空気を導入しない」
• レビュイーが育っていく道筋を敷く
–ルーキーだからレビュアーになれないわけではない
60
ご清聴ありがとうございました
61