Google's Engineering Practices documentation は読んでいてためになったので気になった部分をメモ。
The Standard of Code Review
- perfect じゃなくても code health が良くなれば approve
- (perfect を求めない一方で) reviewer は改善点をコメントできるべきで、ただし重要じゃなくて author が無視していいなら nits を付けると良い
Mentoring
- 知識の共有は code health 向上にも良い
- 一方で教育目的のコメントなら nits を付けるとか、この CL で必ずしも解決しなければならないことではないと伝わるように心掛ける
Principles
- 意見とか個人の嗜好 < 事実とかデータ
- デザインはスタイルとか個人の嗜好にはならない（むずい）
- たぶん、 reviewer と author でデザインの選択で意見が割れる時もあるけど、事実としていずれも等しく有効なものであると示されたなら（reviewer の嗜好とは反しても） author の変更は approve しないといけないという感じか？
Resolving Conflicts
- 合意が取れないからといって CL を放置しないように
What to look for in a code review
Design
- 一番大事
- 機能追加するに良いタイミングか？
Functionality
- 並列プログラミングは難しいので注意 （ので使わない理由にもなりえる）
Complexity
- 読むの難しい、つまり変えるときに bug になりやすい
- over-engineering (必要以上の一般化や今はいらない機能) に注意する
Tests
- テストは一緒の CL に
- main には入らないからといって complexity を許容しない
Naming
- 良い名前はそれを表現するのに十分に長くて、読みづらくないように長すぎない（むずい）
Comments
- コメントというのは普通 explain why するもので explain what してたらそれはコード読んでもわからないということなのでコードの方をシンプルにした方がいいのではないか
- note: ドキュメンテーション != コメント
Style
- 色々問題引き起こすのでスタイルの変更を別の変更に混ぜないこと
Documentation
Every Line
Context
Good Things
- コードレビューは間違いに注視しがちなので褒めることも忘れない
Summary
Navigating a CL in review
Summrary
Step One: Take a broad view of the change
- CL description をまず見て reject するならすぐに返信する
- 対案を入れる
- 礼儀正しくする
Step Two: Examine the main parts of the CL
- 大きすぎたら CL 分けてもらうように頼む
- major なデザインの問題があったらすぐに返信すべき
Step Three: Look through the rest of the CL in an appropriate sequence
Speed of Code Reviews
Why Should Code Reviews Be Fast?
- チーム全体のスピードが落ちるから
- 不満が溜まるから
- code health もリファクタとかし辛くなって影響でてくるから
How Fast Should Code Reviews Be?
- きたらすぐに返事を返す、1 営業日が最大
Speed vs. Interruption
- 仕事に集中してる時は中断しない
- 中断すると戻るのに時間がかかるという話
- break したら返信しよう
Fast Responses
- CL に対して終わりまでが早くなるより個々のレスポンスが早くなる方が重要
Cross-Time-Zone Reviews
LGTM With Comments
- 時間の節約のためのコメント付き LGTM
- 適切に対処してくれそうだと確信できていれば
- minor で必ずしも変更しなくていいなら
Large CLs
- 時間がない、時間かかるなら分けてもらうよう頼む
Code Review Improvements Over Time
Emergencies
How to write code review comments
Summary
Courtesy
- 人に対してではなく、コードに対してコメントするといい
Explain Why
- 何故コメントしたのかが伝わるように
Giving Guidance
- 問題を伝えて解決してもらうのか、より直接的な指示を与えるかはバランスをとる
- 一番の目的は CL をベストにすること
- 二番目に開発者のスキル向上
Accepting Explanations
- レビューツール上のコメントだけでは将来の開発者には分からないので、説明を要求されたら普通はもっと分かるようにコードを変更する必要がでてくる
Handling pushback in code reviews
Who is right?
- 同意が得られなかったら author が正しいか考える
- それでも正しいと思えなかったら、より説明を加える
- author からの返信を理解しようとしている姿勢
- その上で何故変更を求めているかということ
Upsetting Developers
- ちゃんと丁寧にしてたら大抵は大丈夫なはず
Cleaning It Up Later
- 後でやるから LGTM くれというのは大抵はやらないことになってしまうから、今すぐやる方がいい
General Complaints About Strictness
- レビューをきつくすると不満が出るがスピードが早ければ大抵は緩和される
Resolving Conflicts