コードレビューのやり方とか目的とか
コードレビューを成功させるためにCTOが考えるべき7つのこと
Revieweeの理解レベルを考える
プログラミングそのものへの理解レベル
要求仕様に関する理解レベル
コードレビューの目的が何であるかコンセンサスを取る
「コードレビューが終了したときに、どういう状態になっていれば成功なのか」
人を責めない
いいところは褒める
コードレビューが重要な理由 (時間の節約にもなります!)
Knowledgeの共有
SPOFなチームメンバーがいなくなる
新しいメンバーの教育
「アトラシアンでは、多くのチームがコードベースへ投入する前にすべてのコードを 2 回レビューしています。」
「アップストリームへのマージ前にコードレビューを必要とすることで、レビューされていないコードがマージされてしまうことを防ぎます。」
第2章 脆弱性回避策とソフトウェア開発工程 ソースコードレビュー (IPA セキュアプログラミング講座)
セキュリティ脆弱性のきざしを読み取る
レビューには4つの段階が存在する
下読み：全体の構造を把握する
成熟度点検：命名、アサート、モジュール分割などいわゆるCode Qualityを点検する
機能点検：コードが所望の機能を実現するか確認する
脆弱性点検：脆弱な関数呼び出しやロジックがないか確認する
新卒の私が受けたコードレビューを振り返ってみた
Sansanにおけるコードレビューの要点
コードの可読性
命名
コンポーネントの責務
PRの作り方
コードレビューのやり方、基礎の基礎 - コード改善に重要なレビューの基本的な考え方を学ぼう
2段階レビューへの言及
1回目はコードに欠陥がないかを洗い出す
2回目は代替案がないか考える
コードレビューは目的意識が共有されず、見様見真似で続けられていることが多い
目的を決める
ポイント
バグがなさそうか
コーナーケース
拡張性チェック
Code Review Best Practices
Motivation
Committerにとって、コードが誰かに認知されているという確信がモチベーションに繋がる
Knowledge sharing
コードの一貫性
バグの発見
レビューのタイミング
CIが通ってからマージされるまでの間
あまりにも変更が大きい場合、一つのfeature branchからさらに細かくbranchを生やして（新しくチケットは切らずに）段階的に小さいレビューを投げるのも手
レビューが以下のどれかの条件を満たすなら分割を検討する
5ファイル以上が大きく変更されている
書き上げるのに1～2日以上かかっている
レビューに20分以上かかると思われる
レビューに投げる前に自分でテストする
リファクタリングは挙動を変えてはならない
Committerは1人か2人Reviewerを選ぶ。
そのうちの1人はProject leadやSenior engineerであることが多い。
Product ownerは全てのレビューをSubscribeするべき
3人以上のReviewerがいると、意見がまとまらず議論が発散しやすいので避けたほうがよい
レビューのポイント
コードはAuthorの意図を反映しているか？
自分ならどう実装するか？
既存ライブラリは使えないか？
標準的なパターンに沿ってコードが書かれているか？
外部ライブラリ・サービスへの依存は変わっていないか？
コードは読みやすいか？
テストは書いてあるか？
既存のテストは壊れうるか？
Integration testが必要か？
コメントやコミットメッセージにもフィードバックを残す
外部ドキュメントは更新されているか？
レビューコメントには必ず返事を返す
Lessons From Google: How Code Reviews Build Company Culture
バグが見つかるなら早い段階で見つかったほうが良い
https://gyazo.com/6aed61b8add146b9649a984914ab730d
コードレビューが実際にバグを見つけられるかは議論の余地がある
"In fact, static analysis tools and unit tests are much better than reviews at ratcheting up and maintaining correctness in individual pieces of code over time."
コードレビューの効果
会社としてコードやfeatureの精査が重要であると思っていることのメッセージになる
チームのレベルを上げる
チームワークを強化する
逆説的だが、レビューによって生じる摩擦を解決することで、チームとしての働き方を学べる
セキュリティ意識が高まる
Frames social recognition for reviewees
Code review guidelines - CodeProject
自分のコードをレビューする意識
You are not your code
間違いが必ず起きるということを認める
自分の信念に従って戦い、負けは潔く認める
codeing standardに従う
人ではなくコードを批判する
自分より知識のない人にも敬意と忍耐をもって接する
レビューは問題解決の場ではない
質問形でレビューする
"Why"を使わず具体的な"What is reasoning..."等で言い換える
ちゃんと褒める。Developerも人なので、自分で書いたコードは心に近い所にある。
急ぐ必要はないが、Revieweeをあまり待たせないようにする
レビュー項目にSeverityをつける。Reviewerは優先度の高い項目からレビューしていく。
チェックリストを使う（本文最後にチェックリスト例がある）
Humanizing Peer Reviews
"Peer Reviews" と書いてはいるがコードレビューの話（2002年の論文なので、おそらく用語が確立していない）
なぜ人はレビューを嫌がるのか？
単に新しいことをしたくない
間違いが見つかった時に罰されることを恐れている
コードを見せることによりプライバシーが侵害されていると感じる（コードと自我が分離できていない）
マネジメントが正しいメッセージを発することで、これらの障害を取り除いていく必要がある
レビュー結果を評定に使わない
レビューのポリシーと期待するものを明確にする
Why Review Code?
バグの早期発見
Mainteinabilityの維持
Knowledge sharing
Best Practices for Code Review
レビュー量は400行以内に抑える
レビューのペースは500行 / 時間までで抑える
一度のレビュー時間は60分以内に抑える
ゴールを設定し計測する
Authorはあらかじめコードにコメントしておく（レビューじゃなくてソースのコメントにしたほうがいいのでは？）
Positiveなコードレビュー文化を醸成する
How to do a code review | eng-practices
"In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect."
Reviewerは常にレビュー内容と変更そのものの重要性を天秤にかける必要がある
「完全なコード」は存在しない。Continuous Improvementを意識する。
（最重要）全体の設計
正しく機能を実現しているか
不必要に複雑ではないか
テストが書かれているか
コメント
コメントは「なぜこのコードが必要か」を書く。「何をしているか」ではない。
正規表現や複雑な数式などは例外的に「何をしているか」を書く必要がある。
命名、スタイル、既存のコードとの一貫性、ドキュメント
変更が作られた文脈
システム全体のコードクオリティを下げるような変更は拒否してよい
レビューの手順
1. 広い文脈を理解する。レビューのdescriptionを読む。よりよい代替案があるならこの時点で提案する。
2. 変更のメインの部分をレビューする。大きな問題がある場合、他の部分をおいてもまずコメントを残す。
3. 枝葉の部分をレビューする。
コードレビューが遅いと開発速度が落ちるだけでなく、レビューそのものが忌避されるようになる
コードレビューには1日以内に反応する
ただし、他のことに集中してるときに割り込むほどではない
全てを終わらせることにこだわるより、早く返事を返すほうがRevieweeにとって良い
時間がないなら、Navigating a CL in reviewの途中の段階で一時的にコメントをつけておく
レビューが大きすぎるなら細かくするように頼んでよい
客観的なコメントを心掛ける。"You"を使わない
なぜこのコメントを書いているか説明する
何をしてほしいか手取り足取り教える必要はないが、有用なアドバイスは積極的に書く
レビュー内容について合意が取れないときにどうするか
Clean up laterは大抵うまくいかない。忘れ去られてしまうため。
Changelistが原因ならレビューの中で直す。
既存の複雑性が原因の場合、チケットを切って即座にアサインする。
The CL author’s guide to getting through code review
1行サマリーと詳細な説明を書く
具体例が書いてあって便利
レビューの簡単さ、バグの混入しづらさ、ロールバックの容易さなどの観点から、Changelistは可能な限り小さいことが望まれる
最適な大きさは「それ自体で完結しているChangelist」
ひとつの問題だけを解決している
テストは含んでよい
マージされた後もシステムが壊れない
APIの実装に付随して使用例なども含むことがある
リファクタリングはレビューを分ける
Revieweeの心得
レビューコメントを人格攻撃として捉えない
Reviewerがコードをちゃんと理解できないのであれば、他の人も理解できない可能性が高い。そういうときはおとなしく直す。
レビューを受けたらまず「Reviewerは正しいかどうか」を考える。自分のコードが100％正しいと思っていたとしても、必ずレビューコメントを吟味する。
依然として自分が正しいと思うなら、そう思う理由をきちんと説明する。Reviewerとの間にきちんと合意を形成する。