コードレビューを成功させるためにCTOが考えるべき7つのこと コードレビューの目的が何であるかコンセンサスを取る 「コードレビューが終了したときに、どういう状態になっていれば成功なのか」 コードレビューが重要な理由 (時間の節約にもなります!) 「アトラシアンでは、多くのチームがコードベースへ投入する前にすべてのコードを 2 回レビューしています。」 「アップストリームへのマージ前にコードレビューを必要とすることで、レビューされていないコードがマージされてしまうことを防ぎます。」 第2章 脆弱性回避策とソフトウェア開発工程 ソースコードレビュー (IPA セキュアプログラミング講座) 成熟度点検：命名、アサート、モジュール分割などいわゆるCode Qualityを点検する 脆弱性点検：脆弱な関数呼び出しやロジックがないか確認する コードレビューのやり方、基礎の基礎 - コード改善に重要なレビューの基本的な考え方を学ぼう コードレビューは目的意識が共有されず、見様見真似で続けられていることが多い Code Review Best Practices Committerにとって、コードが誰かに認知されているという確信がモチベーションに繋がる あまりにも変更が大きい場合、一つのfeature branchからさらに細かくbranchを生やして（新しくチケットは切らずに）段階的に小さいレビューを投げるのも手 レビューが以下のどれかの条件を満たすなら分割を検討する Committerは1人か2人Reviewerを選ぶ。 そのうちの1人はProject leadやSenior engineerであることが多い。 Product ownerは全てのレビューをSubscribeするべき 3人以上のReviewerがいると、意見がまとまらず議論が発散しやすいので避けたほうがよい 外部ライブラリ・サービスへの依存は変わっていないか？ コメントやコミットメッセージにもフィードバックを残す Lessons From Google: How Code Reviews Build Company Culture コードレビューが実際にバグを見つけられるかは議論の余地がある "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 "Why"を使わず具体的な"What is reasoning..."等で言い換える ちゃんと褒める。Developerも人なので、自分で書いたコードは心に近い所にある。 急ぐ必要はないが、Revieweeをあまり待たせないようにする レビュー項目にSeverityをつける。Reviewerは優先度の高い項目からレビューしていく。 チェックリストを使う（本文最後にチェックリスト例がある） "Peer Reviews" と書いてはいるがコードレビューの話（2002年の論文なので、おそらく用語が確立していない） コードを見せることによりプライバシーが侵害されていると感じる（コードと自我が分離できていない） マネジメントが正しいメッセージを発することで、これらの障害を取り除いていく必要がある Best Practices for Code Review Authorはあらかじめコードにコメントしておく（レビューじゃなくてソースのコメントにしたほうがいいのでは？） 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. 変更のメインの部分をレビューする。大きな問題がある場合、他の部分をおいてもまずコメントを残す。 コードレビューが遅いと開発速度が落ちるだけでなく、レビューそのものが忌避されるようになる ただし、他のことに集中してるときに割り込むほどではない 全てを終わらせることにこだわるより、早く返事を返すほうがRevieweeにとって良い 時間がないなら、Navigating a CL in reviewの途中の段階で一時的にコメントをつけておく レビューが大きすぎるなら細かくするように頼んでよい 何をしてほしいか手取り足取り教える必要はないが、有用なアドバイスは積極的に書く レビュー内容について合意が取れないときにどうするか Clean up laterは大抵うまくいかない。忘れ去られてしまうため。 Changelistが原因ならレビューの中で直す。 既存の複雑性が原因の場合、チケットを切って即座にアサインする。 The CL author’s guide to getting through code review レビューの簡単さ、バグの混入しづらさ、ロールバックの容易さなどの観点から、Changelistは可能な限り小さいことが望まれる 最適な大きさは「それ自体で完結しているChangelist」 Reviewerがコードをちゃんと理解できないのであれば、他の人も理解できない可能性が高い。そういうときはおとなしく直す。 レビューを受けたらまず「Reviewerは正しいかどうか」を考える。自分のコードが100％正しいと思っていたとしても、必ずレビューコメントを吟味する。 依然として自分が正しいと思うなら、そう思う理由をきちんと説明する。Reviewerとの間にきちんと合意を形成する。 