我在代码审查中犯的错误以及我现在的做法

我最近看到了David K. Piano关于代码审查的推文：

这让我想起了几年前审查拉取请求时发表的评论：

• 按照“我认为正确的随机顺序”对导入内容进行重新排序
• 不要忘记末尾的逗号！
• 始终使用 X
• 永远不要使用 Y
• 避免做 Z

我经常会重复别人过去告诉我的或者通过阅读类似这篇博客文章学到的“最佳实践”，然后盲目地遵循它们，而没有思考为什么我这样做。

然而，这些事情并没有修复任何缺陷，也没有给产品增加任何价值。我只是在小事上吹毛求疵，让大家的生活变成了一场噩梦。

大多数注释都可以用 ESLint 和 Prettier 轻松修复。有些注释无法修复，但实际上根本没用。我的做法可能不同，但这并不意味着我的方法更好。正如 David Piano 所说：“如果不是自动化的（例如，通过 linter 或格式化程序），你要么手动执行是在浪费时间，要么规则太武断。”

如果我能回到过去，给过去的自己一些建议，我会说：“嘿，伙计，放轻松，冷静下来，专注于重要的事情。”

但什么是重要的？

根据我目前的经验，代码审查有三个用途：

1. 避免错误
2. 学习
3. 教学/指导

当我们泛泛地评论“永远不要使用 Y”时，我们既没有避免 bug，也没有将其作为学习或教学的机会。相反，我们应该针对当前的情况进行更具体的说明：

我认为我们不应该在这里使用 Y，因为请解释一下你的理由。它可能会导致类似问题示例的错误。我们可以通过执行解决方案示例来解决这个问题。

我现在该怎么办？

根据经验法则，我尽量记住以下内容：

• 我的评论有助于预防错误吗？如果是的话，这也是一个教学的机会。因此，我会尝试回答以下问题：它能预防什么错误？为什么会发生错误？我们该如何修复它？
• 我看不懂这段代码。那么，这是一个学习的机会：“我不确定我是否理解了这部分。请问您能添加一些注释来解释一下您的推理吗？”
• 我理解这段代码，它不会导致任何错误，但我会使用不同的方法。我要么不去管它，因为它不会改变最终结果，要么把它当作一个学习和教学的机会：“你有没有考虑过描述一下你的方法？我认为这可以帮助我们在这里添加一些你的方法的好处/指标。你的想法是什么？” 这样，我既可以教授我的方法（以防对方还不熟悉它），也可以了解不使用它背后的原因（以防对方熟悉我的方法，但有我不了解的不使用它的理由）。

现实生活中的例子

前段时间，我遇到了一个 PR，它在 for 循环中使用 async/await 获取数百万个项目：

for (let post of posts) {
  await doSomething(post.id);
}

我的代码审查是这样的：

我认为我们不应该在这里使用这个 for 循环，因为它会阻塞循环。每条帖子都必须等待前一个请求完成后才能发出另一个请求。这会使请求变慢（如果我的计算有误，请随时运行基准测试并纠正我）。我们可以通过执行以下操作来解决这个问题：

const batchActions = posts.map(post => doSomething(post.id));
await Promise.all(batchActions);

这样，我们就可以同时执行这些操作，从而提高响应速度。请告诉我这是否合理，或者我是否遗漏了其他内容。

我发表评论的意图是：

• 避免可能损害应用程序的性能错误。
• 解释为什么会发生该问题（阻塞循环）。
• 请举例说明我们可以采取哪些措施来解决该问题。
• 如果我的指标有误，或者我没有发现导致他们使用该解决方案的其他问题，请将其保留以供质疑。

总结

确保你的代码审查意见切实可行，并能为产品增值。不要浪费时间在那些不会改变最终结果的事情上。它是否按预期运行，并且通过了你的持续集成 (CI) 流程？然后，继续下一步。

代码审查旨在改进产品并加快开发速度。如果代码审查拖慢了你的进度，损害了团队的利益，那么也许是时候重新审视你的实践了。归根结底，“放松一下，享受这个过程吧。”😉

在 Twitter 上关注我