Google 是如何做 Code Review 的?| 原力计划( 二 )


  • 软件设计从来不是纯粹的代码风格或是个人偏好问题 , 它们是基于一些应当被权衡的规则而不仅仅是个人倾向 。有时候也会有多种有效的选项 , 如果开发者能证明(通过数据或者原理)这些方法都同样有效 , 那么评审者应该接受作者的偏好 , 否则应该遵从软件设计标准 。
  • 如果没有其他的规则使用 , 只要保证不会影响系统的健康度 , 评审者可以要求开发者保持和现有的代码库一致 。
  • 解决代码冲突
    如果Code Review中有任何冲突 , 开发人员和评审人员都应该首先根据开发者指南和评审者指南中其他文档的内容 , 尝试达成一致意见 。
    当很难达成一致时 , 开发者和评审者不应该在Code Review评论里解决冲突 , 而是应该召开面对面会议或者找个权威的人来协商 。(如果你在评论里协商 , 确保在评论里记录了讨论结果 , 以便日后其他人翻阅 。)
    如果这样都解决不了问题 , 那解决问题的方式就应该升级了 。通常的方式是拉着团队一起讨论、让团队主管来权衡、参考代码维护者的意见 , 或者让管理层来决定 。不要因为开发者和评审者不能达成一致而把变更一直放在那里 。
    Code Review应该关注什么?
    注意:当我们考虑以下点时 , 应当始终遵循Code Review标准 。
    设计
    Code Review中最重要的一个点就是把握住变更中的整体设计 。变更中各个部分的代码交互是否正常?整个改动是否属于你负责的代码库?是否和你系统中其他部分交互正常?现在是否是添加整个功能的恰当时间?
    功能性
    开发者在这个变更中想做什么?开发人员打算为该代码的用户带来什么好处?(这里”用户“是指受变更影响到的实际用户 , 和将来会使用到这些代码的开发者)
    重要的是 , 我们希望开发者能充分测试代码 , 以确保代码在Code Review期间正常运行 。但无论如何 , 你作为审查者也要考虑一些特殊情况 , 像是有些并发问题 。站在用户的角度 , 确保你正在看的代码没有bug 。
    你可以验证变更 , 尤其是在有面向用户的影响时 , 评审者应该仔细检查整个变更 。有时候单纯看代码很难理解这个变更如何影响到用户 , 这种情况下如果你不方便自己在CL中打补丁并亲自尝试 , 你可以让开发者为你提供一个功能性的demo 。
    另一个在Code Review时需要特别关注的点是 , CL中是否有 并发编程 , 并发理论上可能会导致死锁或资源争抢 , 这种问题在代码运行时很难被检测出来 , 所以需要有人(开发者和评审者)仔细考虑整个逻辑 , 以确保不会引入线程安全的问题 。(所以除了死锁和资源争抢之外 , 增加Code Review复杂度也可以成为拒绝使用多线程模型的一个理由 。)
    复杂性
    变更是否比预期的更复杂?检测变更的每个级别 , 是否个别行太复杂?功能是否太复杂?类是否太复杂?”复杂“意味着代码阅读者很难快速理解代码 , 也可意味着开发者尝试调用或修改此代码时可能会引入bug 。
    一个典型的复杂性问题就是 过度设计 , 当开发者让代码变得更通用 , 或者增加了许多当前不需要的功能时 , 评审者应该额外注意是否过度设计 。鼓励开发者解决现在遇到的问题 , 而不是揣测未来可能遇到的问题 。未来的问题应当在遇到时解决 , 到那个时候你才能看到问题本质和实际需求 。
    测试
    开发人员应当进行单元测试、集成测试或端到端测试 。一般来说 , 变更中应该包含测试 , 除非这个变更只是为了处理紧急情况 。
    确保变更中的测试是正确、合理和有用的 。因为测试本身无法测试自己 , 而且我们很少会为测试编写测试 , 所以必须确保测试是有效的 。
    如果代码出了问题 , 测试会失败吗?如果代码发生改动 , 它们会误报吗?每一个测试都有断言吗?是否按照不同的测试方法对测试进行分类?
    记住 , 不要以为测试不是二进制中的一部分就不关注其复杂度 。
    命名
    开发人员是否使用了良好的命名方式?好的命名要能够充分表达一个项(变量、类名等)是什么或者用来做什么 , 但又不至于让人难以阅读 。


    推荐阅读