审查沟通技巧

代码审查不仅是技术活动，更是团队沟通的重要环节。良好的沟通能够提升审查效率，维护团队氛围，促进知识共享。本章将介绍审查中的沟通技巧和最佳实践。

沟通原则

对事不对人

这是代码审查沟通的首要原则。审查意见应该针对代码本身，而不是编写代码的人。

不好的表达：

"你怎么又犯这种错误？" "你的代码逻辑太混乱了。" "这写得什么玩意儿？"

好的表达：

"这段逻辑在处理边界情况时可能有问题，建议添加对空值的检查。" "这个函数的职责不够单一，建议拆分成两个函数。" "这个实现方式可能有性能问题，建议考虑使用缓存。"

具体且可操作

模糊的评论会让开发者困惑，不知道该如何改进。审查意见应该具体说明问题所在，并提供可行的解决方案。

不好的表达：

"这里有问题。" "代码质量不高。" "需要优化。"

好的表达：

"这里的递归实现对于深层嵌套的数据可能会导致栈溢出。建议改为迭代实现：\njava\npublic void processNode(Node root) {\n Stack<Node> stack = new Stack<>();\n stack.push(root);\n \n while (!stack.isEmpty()) {\n Node node = stack.pop();\n process(node);\n \n if (node.right != null) stack.push(node.right);\n if (node.left != null) stack.push(node.left);\n }\n}\n"

解释原因

说明为什么某个做法不好，以及改进后的好处，能够帮助开发者理解并记住这个知识点。

不好的表达：

"不要用魔法数字。"

好的表达：

"这里的 86400 是一个魔法数字，表示一天的秒数。建议提取为命名常量，这样：\n1. 提高代码可读性，其他开发者一眼就能明白含义\n2. 便于维护，如果一天秒数定义改变，只需修改一处\n3. 避免重复，如果多处使用，可以复用常量\n\n建议修改为：\njava\nprivate static final int SECONDS_PER_DAY = 86400;\n\nint days = totalSeconds / SECONDS_PER_DAY;\n"

认可好的代码

代码审查不只是找问题，也应该认可和表扬好的实现。这能够激励开发者，营造积极的审查文化。

示例：

"这个错误处理设计得很好，使用了自定义异常并保留了原始异常信息，便于问题排查。"

"使用 Optional 来处理可能为空的情况是个很好的选择，避免了空指针异常。"

"这个函数拆分得很合理，每个函数职责单一，易于测试和维护。"

意见分级表达

阻塞性问题（Blocking）

必须修复才能合并的问题，通常涉及安全、功能缺陷或严重性能问题。

标记方式：

🔴 阻塞：这里存在 SQL 注入漏洞，必须使用参数化查询。

当前代码：
String query = "SELECT * FROM users WHERE id = " + userId;

建议修改为：
PreparedStatement ps = conn.prepareStatement(
    "SELECT * FROM users WHERE id = ?"
);
ps.setInt(1, userId);

使用场景：

• 安全漏洞
• 功能缺陷
• 严重性能问题
• 破坏向后兼容性

建议性问题（Suggestion）

建议采纳但不是必须的问题，通常涉及代码优化、设计改进等。

标记方式：

💡 建议：这里的循环可以替换为 Stream API，代码会更简洁。

当前实现：
List<String> result = new ArrayList<>();
for (User user : users) {
    if (user.isActive()) {
        result.add(user.getName());
    }
}

建议改为：
List<String> result = users.stream()
    .filter(User::isActive)
    .map(User::getName)
    .collect(Collectors.toList());

这样代码更简洁，也更容易理解。

使用场景：

• 代码优化建议
• 设计改进建议
• 更好的实现方式
• 性能优化建议

可选性问题（Nitpick）

仅供参考的小问题，通常涉及代码风格微调或个人偏好。

标记方式：

📝 可选：变量名 `data` 可以更具体一些，比如 `userList`，这样更容易理解变量的用途。

使用场景：

• 代码风格微调
• 个人偏好
• 非关键优化
• 命名建议

问题澄清（Question）

对代码逻辑或设计选择有疑问，需要开发者解释。

标记方式：

❓ 疑问：这里为什么选择使用乐观锁而不是悲观锁？这个场景下并发冲突的概率高吗？

使用场景：

• 理解设计意图
• 确认业务逻辑
• 了解技术选型原因

处理分歧

理解对方观点

当对审查意见有不同看法时，首先要确保理解对方的担忧。

示例对话：

审查者：这里的实现方式在高并发场景下可能会有性能问题。

开发者：我理解你的担忧。你是担心数据库连接池会被耗尽，对吗？

审查者：是的，特别是当外部服务响应慢的时候，可能会导致大量连接被占用。

解释你的考虑

说明自己选择当前方案的原因，帮助审查者理解上下文。

示例对话：

开发者：我选择同步调用是因为：

1. 这个接口的调用频率不高（每天约 100 次）
2. 业务上需要实时返回结果
3. 外部服务的平均响应时间在 200ms 以内

不过你的担忧是有道理的，如果外部服务变慢确实会影响系统。

寻求共识

提出折中方案或下一步行动计划。

示例对话：

开发者：要不我们先按当前方案实现，同时：

1. 添加超时控制（5 秒）
2. 添加熔断机制
3. 添加监控告警
4. 记录一个技术债务，后续评估是否需要改为异步处理

审查者：可以，这样比较稳妥。记得在代码里加上 TODO 注释说明。

升级处理

如果双方无法达成一致，可以：

1. 引入第三方：邀请技术负责人或其他资深开发者参与讨论
2. 记录决策：将讨论结果和决策理由记录下来
3. 优先合并：避免长期阻塞，可以合并后持续观察

回复技巧

作为代码作者

确认修复

已修复：在 commit abc123 中修改。

说明：采用了建议的方案，将循环改为 Stream API，代码确实更简洁了。

解释原因

关于这个问题，我考虑过使用缓存，但是：
1. 这个数据变化比较频繁
2. 查询本身很快（< 10ms）
3. 缓存会增加系统复杂度

所以我认为当前方案更合适。你觉得呢？

请求澄清

关于这个建议，我不是很理解。能否详细说明一下：
1. 具体是哪个地方有问题？
2. 推荐的做法是什么？
3. 有什么好处？

谢谢！

作为审查者

确认修复

修复看起来不错！Stream API 的使用很恰当。

一个小建议：可以考虑使用方法引用 `User::getName` 替代 lambda，代码会更简洁。

接受解释

明白了，你的考虑很有道理。在当前场景下确实不需要缓存。

建议添加一个注释说明这个决策，方便后续维护者理解。

坚持观点

我理解你的考虑，但是我仍然认为这个问题需要解决。

原因是：
1. 虽然当前调用频率不高，但业务在快速增长
2. 外部服务的不稳定性是我们无法控制的
3. 一旦出现问题，影响面会很大

建议我们先实现熔断机制，这样既能满足当前需求，又能防范风险。你觉得如何？

避免常见沟通陷阱

陷阱一：过于主观的评论

不好的表达：

"我不喜欢这种写法。" "这样写不好看。"

好的表达：

"这种写法在团队协作中可能会引起混淆，因为变量名 data 不够具体，建议使用更具描述性的名称，如 userList 或 activeUsers。"

软件设计几乎从来不是单纯的风格问题或个人偏好，而是基于基本原则的。如果作者能证明多种方法同样有效，审查者应该接受作者的偏好。

陷阱二：要求完美

追求完美会严重阻碍开发进度。没有"完美"的代码，只有"更好"的代码。

不好的做法：

对每个细节都提出修改要求，直到代码达到"完美"状态才批准。

好的做法：

只要代码确实能改善系统的整体健康状况，就应该批准合并。不重要的改进建议可以标注为 "Nit:" 或 "可选："，让作者自行决定是否采纳。

审查者应该始终可以留下评论表达某些地方可以改进，但如果不重要，应该用 "Nit:" 前缀，让作者知道这只是打磨建议，可以选择忽略。

陷阱三：忽视代码上下文

在不了解上下文的情况下给出评论，可能导致误解或不切实际的建议。

不好的做法：

"这个设计太复杂了，应该简化。"

（但可能不了解历史原因或业务约束）

好的做法：

"这个设计看起来比较复杂。能否解释一下为什么需要这样设计？是否有历史原因或特殊约束？"

如果对某段代码不理解，请作者解释通常应该导致他们重写代码使其更清晰，而不是只在审查工具中解释。

陷阱四：过度设计的要求

要求在当前变更中解决所有潜在问题，即使这些问题与当前变更无关。

不好的做法：

"既然你改了这个文件，顺便把整个文件都重构一下吧。"

好的做法：

"这个文件确实需要重构，但这超出了当前 PR 的范围。我创建了一个技术债务工单 #123 来跟踪这个任务。"

给予有效指导

平衡指出问题与提供指导

一般来说，修复 CL 是开发者的责任，而不是审查者的责任。审查者不必为开发者提供详细的解决方案设计或编写代码。

但这并不意味着审查者应该不提供帮助。在指出问题和提供直接指导之间应该取得适当的平衡。

只指出问题：

"这里有个 bug。"

（开发者可能不知道如何修复）

提供具体指导：

"这里在处理空值时可能会抛出 NullPointerException。建议添加空值检查：

if (user != null && user.getAddress() != null) {
    return user.getAddress().getCity();
}
return "Unknown";
```"

让开发者自己决定解决方案通常有助于开发者学习，也使代码审查更容易进行。同时，开发者比审查者更接近代码，可能会找到更好的解决方案。

正面反馈同样重要

人们不仅从"可以做得更好"中学习，也从"做得好"的正向反馈中学习。如果在 CL 中看到喜欢的东西，也应该评论出来！

示例：

"这个抽象做得很好，把复杂的逻辑封装得很清晰。" "测试覆盖很全面，特别是边界条件的测试。" "这个重构让代码可读性提高了很多。"

和所有评论一样，说明你为什么喜欢这些东西，鼓励开发者继续这种好的实践。

接受解释

何时接受解释

如果审查者要求开发者解释某段不理解的代码，通常应该导致开发者重写代码使其更清晰，而不是只在审查工具中解释。

只在代码审查工具中的解释对未来代码读者没有帮助。

例外情况：

• 审查者正在审查不熟悉的领域，开发者解释的是普通读者已经知道的内容
• 解释的是业务规则或外部约束，这些信息已记录在其他地方

示例

不好的做法：

审查者：这里的逻辑我不太理解。 开发者：这是因为业务规则要求 A 先于 B 执行，所以顺序很重要。 审查者：好的，我明白了。（批准合并）

好的做法：

审查者：这里的逻辑我不太理解，为什么 A 必须在 B 之前执行？ 开发者：（添加注释或重命名变量使逻辑更清晰）已添加注释说明业务规则。 审查者：很好，现在代码更清晰了。

审查速度与文化

及时响应的重要性

代码审查的响应速度直接影响团队的开发效率。长时间等待审查会：

• 阻塞开发者的工作进度
• 降低团队士气
• 影响产品交付周期

建议：

场景	响应时间
收到审查请求	24 小时内响应
紧急修复	2 小时内响应
审查意见回复	24 小时内回复
阻塞性问题修复	优先处理

不要让 CL 长期搁置

如果作者和审查者无法达成一致，不要让 CL 一直挂着。应该：

1. 尝试面对面或视频会议解决冲突
2. 讨论结果记录在 CL 评论中
3. 必要时升级给技术负责人决策

建立健康的审查文化

心理安全感

健康的代码审查文化需要心理安全感：开发者不必担心因代码被批评而受到打击。

团队领导者的作用：

• 以身作则，主动接受审查意见
• 公开认可好的审查行为
• 处理不当的评论方式

持续学习

将审查视为学习机会而非负担：

• 从他人的代码中学习新技术和模式
• 通过审查意见了解最佳实践
• 分享审查中的发现和收获

共同责任

代码质量是团队共同的责任：

• 审查者不是"找茬者"，而是"协作者"
• 作者和审查者共同对代码质量负责
• 鼓励跨团队审查，促进知识共享

小结

良好的代码审查沟通是团队协作的关键：

• 对事不对人：评论针对代码，不针对开发者
• 具体可操作：提供明确的问题描述和改进建议
• 解释原因：帮助开发者理解为什么要改
• 认可好的代码：正面反馈同样重要
• 意见分级：区分阻塞、建议、可选、疑问
• 及时响应：避免让 CL 长时间等待
• 持续改进：追求"更好的代码"，而非"完美的代码"

通过遵循这些沟通原则，代码审查可以成为提升代码质量、促进团队成长的有效工具，而不是开发流程中的负担。

参考资料

• Google Engineering Practices - Code Review
• How to Write Code Review Comments
• The Standard of Code Review