跳到主要内容

审查沟通技巧

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

沟通原则​

对事不对人​

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

不好的表达：

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

好的表达：

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

具体且可操作​

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

不好的表达：

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

好的表达：

"这里的递归实现对于深层嵌套的数据可能会导致栈溢出。建议改为迭代实现：\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。