コードレビューの必要性
-
コードの品質を高める
-
バグのないコードを書き続けるのは不可能
- 思い込みや先入観のない目で見直してもらう
-
-
属人性の低減
- コードを実装者のものからチームのものにする
-
心理的安全性の向上
- 不具合発生時の責任感もみんなで背負う
-
個々人の技術力の向上
- 他人のコードを読むことでも技術は向上する
- レビュアーとレビュイーとでコミュニケーションをとりながら切磋琢磨
Pull Requestを利用したコードレビュー
- 前職ではコードを印刷してレビューしてました 終わっている
Pull Requestとは？
- GitHub, GitLab, BitBucket等だいたいおなじ(名前はちがう)
- 「僕がつくったブランチは問題がないのでmasterブランチにマージしてください」とお願いする作業のこと
GitHub Flow
- remoteからリポジトリをcloneする
- masterブランチからfeatureブランチを切る
- featureブランチ上でコードの改修を行う(適宜commit)
- 改修が一通り完了したらremoteにpushする
- 問題がなければmasterブランチにmergeする
- masterブランチのコードをデプロイする
Git Flow
- Git Flow
git flowコマンドを使えるように、拡張を入れたりするらしい
Pull Requestをつくってみよう
-
GitHubの場合、コミットメッセージの内容からPRをつくれる
- 1行目: title
- 3行目以降: description
- md記法使える
Pull Requestの作り方
title
fixed #123とか書くと、マージされた時対応するissueを閉じてくれる- 途中でレビューしてほしいときは
WIPとか明記するとよい
description
- やったこと
- 対応するissueへのリンク
-
レビュアーの無駄な指摘を減らすために下記もやると親切
- やっていないこと
-
hotfixのときは、「現時点では妥協している点」とかも
- 一次対応後に別途改善が必要なら、後から再度PR送る
inline comment
- コードに行単位でコメントできる
-
妥協点等、レビュアーに突っ込まれそうな点を予めコメントしておくなど
- 無駄な指摘を減らす
-
迷った点等、特に意見がほしい箇所にもコメント
- 【所感】「全部見てください」は経験上かなり苦痛だし集中できない
Pull Requestのコードレビューの流れ
| 実装者 | レビュアー | |
|---|---|---|
| 1. | PRつくる | |
| 2. | コードレビュー依頼する | |
| 3. | コードレビュー行う | |
| 4. | FB | |
| 5. | FB受けて適宜直す | |
| 6. | 2-5繰り返す | |
| 7. | 問題がなければLGTM出す(Looks Good To Me) | |
| 8. | masterブランチにマージする |
-
with nits (nitpick), imo (in my opinion)
- 些細なポイントで気になる点があるので適宜修正してください
- 再レビューは不要です
-
masterへのマージは実装者がやる現場が多いらしい
- 2019/3現在携わっているプロジェクトはレビュアーがマージしている
コードレビューをしよう
コードレビューの観点
-
再優先事項: ユーザーに迷惑をかけない
- 不具合がないこと
- 要求仕様を満たしていること
-
動的型付け特有の挙動
- 比較演算子
-
識別子名の一貫性
- 例えば、PSR-2を守ろう
- PHP Snifferとか使う
-
無駄に複雑なコードを書かない
- ifのネストとか。本当に必要か
-
メソッドの切り出し
- 名前をつけて単一責務に
-
パフォーマンス
- 「0.1秒高速化につき売上1%上昇」 by Amazon
N+1のクエリとかはNG
-
フレームワークに乗っかる
Collectionクラスを使うとか
レビュアーとして気をつけること
-
人格否定をしない
- 心理的安全性
-
敬意
- 良いと思った箇所を褒めるのもよい
-
客観的に
- 「なんか気持ち悪いコード」とかは駄目
実装者として気をつけること
- 真摯に受け止める
-
コミットメッセージをきれいに書く
- コミットは適切な粒度で
- 何をやったのか
- なぜやったのか
コードレビューの難しさ
-
コードレビューの目的や重要視するポイントが現場や時期によりさまざま
-
ベンチャーではスピード重視だったりする
- 【所感】ぼくの前職もそれに近かった。品質担保などをかなぐり捨てて全速力で突っ走っていた
- 大規模FinTechなどでは品質を十二分に気をつける
-
-
目的をはっきりさせること
- さもないと時間ばかりかかる
- コミュニケーションもギクシャクになる
-
レビューは、レビュアーと実装者との「協働」作業
- ペアプロもgood