如果Code Review中有任何冲突 , 开发人员和评审人员都应该首先根据开发者指南和评审者指南中其他文档的内容 , 尝试达成一致意见 。
当很难达成一致时 , 开发者和评审者不应该在Code Review评论里解决冲突 , 而是应该召开面对面会议或者找个权威的人来协商 。(如果你在评论里协商 , 确保在评论里记录了讨论结果 , 以便日后其他人翻阅 。)
如果这样都解决不了问题 , 那解决问题的方式就应该升级了 。通常的方式是拉着团队一起讨论、让团队主管来权衡、参考代码维护者的意见 , 或者让管理层来决定 。不要因为开发者和评审者不能达成一致而把变更一直放在那里 。
Code Review应该关注什么?
注意:当我们考虑以下点时 , 应当始终遵循Code Review标准 。
设计
Code Review中最重要的一个点就是把握住变更中的整体设计 。变更中各个部分的代码交互是否正常?整个改动是否属于你负责的代码库?是否和你系统中其他部分交互正常?现在是否是添加整个功能的恰当时间?
功能性
开发者在这个变更中想做什么?开发人员打算为该代码的用户带来什么好处?(这里”用户“是指受变更影响到的实际用户 , 和将来会使用到这些代码的开发者)
重要的是 , 我们希望开发者能充分测试代码 , 以确保代码在Code Review期间正常运行 。但无论如何 , 你作为审查者也要考虑一些特殊情况 , 像是有些并发问题 。站在用户的角度 , 确保你正在看的代码没有bug 。
你可以验证变更 , 尤其是在有面向用户的影响时 , 评审者应该仔细检查整个变更 。有时候单纯看代码很难理解这个变更如何影响到用户 , 这种情况下如果你不方便自己在CL中打补丁并亲自尝试 , 你可以让开发者为你提供一个功能性的demo 。
另一个在Code Review时需要特别关注的点是 , CL中是否有 并发编程 , 并发理论上可能会导致死锁或资源争抢 , 这种问题在代码运行时很难被检测出来 , 所以需要有人(开发者和评审者)仔细考虑整个逻辑 , 以确保不会引入线程安全的问题 。(所以除了死锁和资源争抢之外 , 增加Code Review复杂度也可以成为拒绝使用多线程模型的一个理由 。)
复杂性
变更是否比预期的更复杂?检测变更的每个级别 , 是否个别行太复杂?功能是否太复杂?类是否太复杂?”复杂“意味着代码阅读者很难快速理解代码 , 也可意味着开发者尝试调用或修改此代码时可能会引入bug 。
一个典型的复杂性问题就是 过度设计 , 当开发者让代码变得更通用 , 或者增加了许多当前不需要的功能时 , 评审者应该额外注意是否过度设计 。鼓励开发者解决现在遇到的问题 , 而不是揣测未来可能遇到的问题 。未来的问题应当在遇到时解决 , 到那个时候你才能看到问题本质和实际需求 。
测试
开发人员应当进行单元测试、集成测试或端到端测试 。一般来说 , 变更中应该包含测试 , 除非这个变更只是为了处理紧急情况 。
确保变更中的测试是正确、合理和有用的 。因为测试本身无法测试自己 , 而且我们很少会为测试编写测试 , 所以必须确保测试是有效的 。
如果代码出了问题 , 测试会失败吗?如果代码发生改动 , 它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?
记住 , 不要以为测试不是二进制中的一部分就不关注其复杂度 。
命名
开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么 , 但又不至于让人难以阅读 。
推荐阅读
- 科比坠机身亡真相大白 科比坠机身亡现场
- 冲泡正山堂金骏眉最佳水温是多少
- 公司电脑无法上网,是mac地址与另一台电脑的冲突了?
- 盘点国内常用CDN公共库
- 兔毛内胆派克服多少钱 派克服内胆是包胆好吗
- CSRF攻击与防御
- IP地址、子网掩码、默认网关、网络地址、广播地址都是什么意思?
- 接吻一直伸舌头的男人是什么性格 男生搂女生腰能感觉出粗细吗
- cgi、fastcgi、PHP-fpm都是什么
- 蜜獾是世界上最厉害的动物吗 什么动物能打败蜜獾