Nolan   in Beijing

# Code Review Guide

# 序言

  • 找到最合适的评审人员
    • 最合适的评审人员是能给出最周全和正确的人，有时候最合适的人不一定这个最初写这段代码的人，有时候一个 PR 里的不同代码片段需要不同的人来审核。如果最合适的人没有时间，也应该将 CL 抄送给他们
  • 目的：确保代码库中的代码质量随着时间推移不断改善
  • 指导原则：
    • 开发人员需要持续对代码进行优化，否则代码库永远不会得到改进
    • 评审人有责任确保所有的提交质量，确保代码库整体不会随着时间流逝而减少
    • 即使代码不够完美，但可以改善整体代码运行状况的，就应该批准（approve）

# 作为评审人：

  • 代码审核需要关注：
    • 设计：代码是否有良好的设计并符合系统架构，遵循系统本身的设计模式
    • 功能：代码是否达到作者的目的，边界情况是否被考虑。如果仅通过代码变更变更很难理解某些变更，可以让开发人员演示
    • 复杂性：代码是否可以更简化；未来的开发者维护该代码时，是否能轻松理解和使用改代码；是否过度设计
    • 测试：是否有正确、设计良好的自动化测试
    • 命名：变量、类名、方法是否清晰易懂。一个好名字足够长，可以充分传达物品的含义或作用，而又不会太长而难以阅读
    • 注释：注释是否清晰、有用。注释应该主要描述这段代码存在的原因或者十四路，而不是这段代码做了什么。如果一段代码需要注释才能解释清楚，那就应该让代码更简洁（正则和复杂的算法例外）
    • 样式：是否遵循 Code Style。如果某次变更既包含了代码风格的格式化和功能变更，则应该分开提PR
    • 文档：是否更新了相关文档
  • 原则：
    • 事实标准和数据高于个人喜好
    • 编码样式是绝对权威
    • 如果开发者可以证明（数据或理论）自己的编码方式更有效，评审人应该接受开发者的偏好
    • 如果没有规则适用，则应该和当前代码库保持一致
    • 在评审中发生意见不一致，且难以达成共识，最常见的解决方法是升级，进行更广泛的团队讨论
    • 如果在评审中看到了好的东西，尤其是很好的回答了评论，应该给予鼓励和赞赏
    • 确保检查要求检查的每一行代码，查看上下文，确保评审是在改善代码运行状况，并称赞开发人员所做的出色工作
    • 评审人不应该追求完美，而是质量的持续改进。所以可以提高系统可维护性，可读性和可理解性的代码变更应该即使批准（approve）
  • 方法：
    • CL（Change List）是否有意义，有没有一个描述准确的说明
      • 如果要拒绝，应该礼貌的提出相关建议
    • 看看变化最重要的地方，是否设计得当
      • 一个文件的变更数量最多通常是主要部分
      • 如果变动太多，需要询问开发者应该重点看哪些，或者要求分成多个 CL
    • 以适当的顺序查看剩余的变化
      • 尝试找出逻辑顺序，或者询问开发人员
      • 不要错过任何变更
  • 评审速度（响应速度）
    • WHY
      • 当评审速度缓慢，整个系统迭代会下降
      • 开发人员会对代码审核产生消极情绪，沮丧甚至抱怨
      • 代码质量可能会受到影响
    • 原则：
      • 花费足够的时间进行评审，确保符合代码审核原则
      • 应该在一个工作日内响应
      • 不要在做写代码之类的重点任务的时候进行代码评审，这样既可能中断开发者或者评审人的重点任务，因为“开发人员在中断之后要花很长时间才能恢复到平稳的开发流程中”，这种消耗比等一会再做代码审核更昂贵
        • 最好在工作某个不忙的时间段进行，比如午休过后、会议之后等
    • 有时候有未解决的评论也可以 approve
      • 审核人对开发者解决剩余的问题有信心
      • 剩下的变更很微小或者影响很小
    • 如果遇到大型的CL，应该要求开发者拆分，实在无法拆分，并且没有足够的时间进行审核，那也应该对总体设计做一些建议
      • 目标之一是在不牺牲代码质量的情况下，不阻碍开发人员后面的工作
    • 遵循这些原则和方法，随着时间推移，评审合作的效率会越来越高，但是要切记，不能因为评审速度在代码质量上做出让步
  • 编写代码评论
    • 原则：
      • 礼貌和尊重，避免说出让人沮丧甚至引起争执的内容
      • 避免反问为什么这么做，而是解释你认为正确的做法
      • 提供指导，而不是直接提供解决方案的详细设计，甚至直接给出代码
        • 这样会帮助开发人员学习，病逝审查过程更快
      • 鼓励开发人员添加注释，而不是只为评审人做出解释
  • 处理开发人员的反驳
    • 要么会不同意您的建议，要么会抱怨您总体上过于严格
      • 有时在提出能被接受的建议前需要几轮的说明和讨论，注意要保持礼貌和耐心
      • 评审人有时候会担心如果坚持要改进，开发人员会感到不高兴，但将来他们会感谢评审人的坚持帮助他们提升了代码质量。并且通常情况下评审人礼貌和耐心的态度根本不会让开发人员烦恼。
    • 有时候开发人希望 CL 尽快通过会说后续会将处理的一些 TODO，但往往会被以往。所以应该坚持开发人员在代码入库前清理完待办事项。“稍后处理”是代码质量退回的常见原因。
    • 如果以前对代码审核比较松懈，突然变得严格，一些开发人员会有抱怨，甚至持续数月才能消失，但最终会看到严格审查的价值。一般提升审核响应的速度可以减少抱怨。

# 作为开发者：

# 编写良好的PR/CL描述

  • 背景：PR/CL的描述存在版本控制的历史永久组陈部分，除了评审人，未来的开发人员也会根据描述来检索相关代码变更信息，如果重要的信息都在代码中而不是描述中，找到相关变更的 RP 会比较困难
  • 方法：
    • 一句话说明PR/CL 做了什么
    • 展开描述信息，对要解决的问题剪短描述，以及为什么是最好的方法，如果有缺陷也应该提及
      • 描述应该包含上下文信息：背景信息，测试结果，设计文档链接
  • Bad Cases:
    • bug fix
    • add patch
    • stage 1
    • init
    • 优化代码
  • 审核期间，PR/CL也会发生重大变化，记得更新描述

# 拆分 Small CL

  • 评审人通常没有上下文，所以对于开发人员可接受的变更量，会让评审人不知所措
  • WHY
    • 评审更快
    • 能进行更彻底的评审
    • 引入错误的可能性较小
    • 如果被拒绝，减少浪费的工时
    • 易于合并
    • 易于设计
    • 并行开发
    • 回滚简单
  • 通常 100 行是合理的大小
  • 变更的文件太多也会影响数量
  • 整个文件的删除当成一行变更
  • 很少会遇到无法拆分 CL 的情况，练习编写小型的 CL，总是可以找到一种将功能分解为小变更的方法
  • 如果少数情况发生了，请事先与评审人沟通，因为会需要长时间的（合格的）审查

# 处理评审人的意见和建议

  • 审查的目的是保持代码库和产品的质量，当评审人对代码提出批评时，请积极看待，不要当成对自己的人身攻击
  • 切勿激怒评审人，这也违反了职业礼节；如果情绪消极或者无法友好的回复，请足够平静以后再回复
  • 如果评审人没有一建设性和礼帽的方式提供反馈，应该线下解释清楚你的看法和你不喜欢的事情，如果还是无法解决，酌情上报。
  • 如果评审人不理解提价的代码，那首先应该澄清代码，或则增加注释。切记，当评审人不理解的时候，未来的维护人员（包括自己）也很难理解。
  • 当收到反馈是，首先考虑的是评审人的建议是否正确。如果无法判断，则需要评审人进一步说明。重要的是达成共识。

# 紧急情况

  • 回滚、修复重大影响的bug、关闭安全漏洞、上线Deadline 等
  • 审阅者应该比其他任何事情更关心审阅的速度和代码的正确性（它是否真正解决了紧急情况？）

# 破窗效应

破窗效应 (opens new window)（英语：Broken windows theory）是犯罪学 (opens new window)的一个理论，该理论由詹姆士·威尔逊（James Q. Wilson）及乔治·凯林（George L. Kelling）提出，并刊于《The Atlantic Monthly》1982年3月版的一篇题为《Broken Windows》的文章。

此理论认为环境中的不良现象如果被放任存在，会诱使人们仿效，甚至变本加厉。一幢有少许破窗的建筑为例，如果那些窗不被修理好，可能将会有破坏者破坏更多的窗户。最终他们甚至会闯入建筑内，如果发现无人居住，也许就在那里定居或者纵火。一面墙，如果出现一些涂鸦 (opens new window)没有被清洗掉，很快的，墙上就布满了乱七八糟、不堪入目的东西；一条人行道有些许纸屑，不久后就会有更多垃圾，最终人们会视若理所当然地将垃圾顺手丢弃在地上。这个现象，就是犯罪心理学中的破窗效应 (opens new window)。

# 参考

https://google.github.io/eng-practices/

序言
作为评审人：
作为开发者：
破窗效应
参考