术语
| Abbr | Description |
|---|---|
| CR | Code Reveiw |
| CL | Change List |
| NIT | Nitpick |
CR的原则
-
Reviewer作为CR的Owner, 有责任保障CL的质量
-
Reviewer不应该追求完美，而应该追求持续改进
- 没有完美的代码，只有更好的代码
- 可以在评论前加上nit:前缀，表示这是一个开发可以暂时忽略的改进点
-
CR的评论应该具有指导意义
- 明确指出错误或需要优化的地方
- 对于优化的内容可以给出自己的建议
-
代码风格应该与现有风格保持一致。如果项目没有统一风格，那就接受开发的风格
- Go代码风格可以参考Go CR Comments
-
有分歧难以达成共识时，发起项目组内部的讨论，带上你的Leader
CR的内容
-
CL的总体设计
- 不同代码段的交互是否有意义
- 此次变更部分与系统的其他模块能否很好地集成
- 是否是合适的添加时机
-
功能验证
- 功能实现符合PRD TD
- 功能性的改动对用户是友好的，不应该有breaking change
- UI的改动是合理且好看的
-
不应该过度复杂和过度设计
- 单行代码是否过于复杂、函数（方法）、结构体等是否过于过来复杂，可读性比较差
- 过度设计：为了让代码过度通用化，超过原有的需求增加一些不需要的新功能
-
有合适的单元测试
- 包含对应的单元测试
- 测试用例是合理且有用的
-
命名规范，简短易懂，看到名字就能知道内容
-
合适的注释，注释应该解释why而不是what
-
遵循统一的代码风格
怎么看CL
-
用宏观的角度来看待改动，查看CL的描述以及它完成的功能
- 该改动是否有意义、合理？如果在第一时间认为不应该发生这种改动，请立即说明原因，并给出对应的建议
-
检查CL的主要部分
- 通常CL内会有文件包含大量的逻辑改动，而它正是CL的主要部分，比如API、核心路径等等
- 如果CL的内容过大，导致无法确定哪一部分是主要的，可以先开发询问或者建议他们拆分CL
- 如果发现主要部分存在设计问题，应该及时评论
-
用合理的顺序查看CL的其余改动
- 试着找出一个有逻辑顺来review其余文件，并确保不会错过任何一个
CR的速度
-
如果你专注于某项工作时，请不要打断自己去做CR
-
及时回复评论比起快速结束这个CR更加重要
-
尽量在开发还在工作时间内回复
-
由于改动过大导致难以review时，可以要求开发将CL拆分成更小的CL
-
某些情况下为加快review速度，可以给出LGTM或Approval的评论
- reviewer确信开发人员会适当地处理所有剩余的评论
- 剩余的评论是微不足道的或它们不需由开发者来处理
-
CR的能力会随着时间进步
- 如果你遵循这些准则，严格对待CR，你会发现整个CR的流程会越来越快
如何写评论
-
当开发人员不同意你的建议时，思考一下谁是正确的，并能给出合理的解释，保持礼貌
-
如果reviewer坚持己见，会让开发者感到沮丧；沮丧很多时候来源于Comment的方式，并非来自于对代码质量的坚持
-
如果CL引入了新的问题，非紧急情况下，必须在提交之前将其处理掉
-
如果现在无法解决reviewer评论的问题，加上TODO注释并链接到刚记录的缺陷
-
发现Bug时，可以将bug链接到对应的comment
评论太严格
提高review的速度会让这些抱怨逐渐消失。这些抱怨可能需要数个月才消失，但最终开发人员会看到严格的review所带来的价值，因为严格的review帮助他们产生的优秀代码。
网友评论