代码审查是开发工作中很重要的一环。但很多有经验的开发者在代码审查时并没有发挥出应有的作用。他们多年的经验最终变成了对代码格式和变量命名的挑剔，而真正的架构风险却被忽略了。结果就是代码看起来很整洁，但基础决策却有问题。

一个擅长代码审查的资深开发者，和只是有经验但不擅长审查的人，区别不在于能发现更多问题，而在于知道哪些问题值得关注，哪些争论值得进行，以及在代码发布前真正需要修改的是什么。

从"为什么"开始，而不是"怎么做"

大多数代码审查都是从看代码开始的。这个顺序其实不对。第一个问题不应该是"这段代码写得好吗？"，而应该是"这段代码有必要存在吗？"。

当看到一个代码提交时，资深开发者的做法是先阅读描述和任务说明，理解要解决的问题，然后思考这是不是正确的解决方案。不是说实现是否正确，而是从根本上说这个解决方案是否应该存在。

有些代码提交中代码很漂亮，测试也很完善，但解决的是错误的问题。比如有人花了两天时间为表格写了自定义虚拟滚动方案，但真正的问题是他们一次性渲染了上万行数据，而不是做分页。代码本身没问题，但思路错了。而大多数审查只关注代码质量，结果是只见树木不见森林。

// 代码提交："添加虚拟滚动来提升表格性能"

// 实现很漂亮，处理了滚动，更新可见范围
function VirtualizedTable({ data, rowHeight = 50 }) {
  const [scrollTop, setScrollTop] = useState(0);
  const containerHeight = 600;

  const visibleStart = Math.floor(scrollTop / rowHeight);
  const visibleEnd = Math.ceil((scrollTop + containerHeight) / rowHeight);
  const visibleData = data.slice(visibleStart, visibleEnd);

  return (
    <div 
      style={{ height: containerHeight, overflow: 'auto' }}
      onScroll={e => setScrollTop(e.target.scrollTop)}
    >
      <div style={{ height: data.length * rowHeight }}>
        <div style={{ transform: `translateY(${visibleStart * rowHeight}px)` }}>
          {visibleData.map(row => <TableRow key={row.id} data={row} />)}
        </div>
      </div>
    </div>
  );
}

这段代码本身其实没问题……但更好的审查评论应该是："在我们做虚拟滚动之前，为什么要一次性加载上万行数据？能不能加分页或者无限滚动？api已经支持分页了，只是我们没在用。"

这样的评论要求你理解更大的系统。你需要知道虚拟滚动会增加复杂度，在有分页这种更简单方案的情况下不应该先引入虚拟滚动。问题不在于代码质量，而是这段代码是否应该存在。

忽略不重要的细节

资深开发者明白，大多数细节并没有看起来那么重要。变量叫userData而不是user？真的那么重要吗？一个可以用三元表达式写的if语句？没什么大碍。用.map()处理十个元素而不是for循环？也没关系。

重要的是知道什么时候应该放过那些不符合个人偏好的写法——不是因为代码质量不重要，而是因为注意力有限，真正重要的问题在别处。

我不是鼓励写糟糕的代码。我是说，我们应该先把注意力放在更重要的事情上。

有一个有用的思维模型：承重墙和非承重墙。有些墙支撑整个结构，有些只是分隔空间。代码也是一样。某些选择是承重的——它们影响性能、安全和长期维护性，值得深入审查。其他只是个人偏好，不会改变结果。资深的代码审查专注于检查承重级别的决策，放过那些非结构性的细节，这样才能把精力花在真正能改变系统的地方。

// 这个值得评论吗？
const isValid = data.name && data.email;

// 对比
const isValid = Boolean(data.name && data.email);

// 对比
const hasName = !!data.name;
const hasEmail = !!data.email;
const isValid = hasName && hasEmail;

三种写法都能完成工作，可能第三种在可读性上稍好一些。但值得为此打断作者、要求修改并重新提交代码吗？值得因为这个小细节而延迟功能发布吗？资深开发者明白大多数可读性争论都是主观的。如果一个解决方案可以理解并且能用，最好放手。把审查精力留给那些真正能产生影响的改动。

寻找可能出问题的地方

重要的代码审查模式是那些能防止生产问题的点。很多资深开发者通常经历过或调试过足够多的问题，因此知道该看什么。

错误处理是最大的一个方面。不是简单检查"你加了try-catch吗"，而是问"如果失败了会怎样？"因为所有东西最终都会失败。API会挂掉、数据库会超时、网络会不稳定、第三方会返回垃圾数据、用户会做出意料之外的操作。

async function getUserProfile(userId) {
  const response = await fetch(`/api/users/${userId}`);
  const data = await response.json();
  return data;
}

这段代码在开发环境和API正常返回JSON时可能工作得很好。但如果API返回500错误呢？或者返回的是错误页面的html而不是JSON？请求超时呢？

初级的审查可能会建议只加一个try-catch。资深的审查会更进一步，问："当这失败时用户体验应该怎样？我们应该显示错误信息吗？重试？使用缓存作为备选方案？哪种失败模式能带来最好的体验？"

关键差别在于关注失败条件下用户的体验，而不仅仅是技术上是否处理了异常。这样的思路有助于避免让真实用户沮丧的问题发生。

关注代码中缺失的部分

最好的代码审查问题通常关注那些缺失的部分。比如：当组件在请求还没完成时被卸载会怎样？用户在结果到达之前跳转到其他页面怎么办？这个函数同时被多次调用会怎样？

回答这些问题需要在脑中模拟代码在各种情况下的行为。这比只检查代码风格或模式更难，但这种思维能发现那些在代码上线后会造成实际问题的细微错误。

function useUserData(userId) {
  const [data, setData] = useState(null);

  useEffect(() => {
    fetchUser(userId).then(setData);
  }, [userId]);

  return data;
}

资深开发者会问："如果userId在请求完成前改变了怎么办？组件在切换到新userId后还会为旧userId更新状态吗？"

知道要注意这个问题后，修复很简单：

function useUserData(userId) {
  const [data, setData] = useState(null);

  useEffect(() => {
    let cancelled = false;

    fetchUser(userId).then(user => {
      if (!cancelled) setData(user);
    });

    return () => { cancelled = true; };
  }, [userId]);

  return data;
}

但只有在你考虑组件生命周期与竞争条件时，才可能发现这一点，而不是仅仅线性地读代码。

知道什么时候应该阻止代码合并

不是所有问题都应该阻止代码合并，资深开发者知道如何评估。有些问题肯定会引发生产问题，有些可能会在以后造成麻烦，还有些只是改进的机会。

安全漏洞、数据丢失风险、影响用户体验的性能问题、内存泄漏，以及会引起错误行为的竞争条件——这些都值得阻止合并。

那些日后代价高昂或难以修改的架构性问题也通常需要阻止。例如，反对一种未来功能难以扩展的做法是明智的，因为合并后重构通常更困难。

可读性改进一般不应阻止。是否缺少测试取决于边界情况的重要性。稍微低效但只在页面加载时运行一次的代码通常可以接受。

判断的标准是：这会导致生产问题吗？这会显著增加以后维护代码的难度吗？如果是，阻止合并；如果不是，提出改进建议但不要阻止进度。

// 值得阻止：
async function deleteUser(userId) {
  await db.users.delete(userId);
  // 缺失：用户的帖子、评论、关系数据怎么办？
  // 这会导致数据不一致
}

// 不值得阻止：
function getUserDisplayName(user) {
  return user.firstName + ' ' + user.lastName;
  // 可以用模板字符串，但这样也能用
}

第一个会导致数据一致性问题，第二个只是风格偏好。阻止第一个，放过第二个。

解释"为什么"

好的评论和优秀的评论差别在于解释为什么它重要。只说"用const替代let"只是条规则，没有上下文。但说"这里的值不会变，用const能向未来的读者清晰地传达这一意图"则解释了背后的原因。

当你解释了为什么，你不仅在给出反馈——你在教学。开发者学到的是原则而不是规则，他们能把这种理解应用到未来的代码中，并且能理解你在乎什么、为什么在乎，这有助于建立共同标准和更好的代码文化。

// 较弱的评论：
"这个应该用useMemo"

// 较强的评论：
"这个计算在每次渲染时都会运行，处理1000多个项目。用useMemo包装，以[items]作为依赖，可以在其他状态变化时避免重新计算。我在本地测试过，渲染时间从80毫秒降到了5毫秒。"

第二条评论不仅告诉你该做什么，还解释了问题、方案和理由。开发者不仅知道有时该用useMemo，还明白什么时候真正重要。

当你不同意某个方案时，这种解释尤其重要。只说"这错了"没有帮助。但说"这个方法在我们下个迭代要加的功能情况下会出问题，因为它假设只有一个活跃用户"则提供了背景，帮助作者看到更大的图景。这样，反馈就成为了引导理解而不仅仅是要求合规。

为可维护性做审查

我们今天提交的代码，是数月后会被别人维护的代码。可能是原作者已经忘记细节，或是新同事要接手。资深开发者在审查时会考虑未来的读者。

他们会问：这段代码是否能自己说明白而无需深层上下文？重要决策是否有注释？细微行为是否被解释？问题不仅是"它做了什么？"，而是"为什么要这样做？"这种方法能保留理解，利于未来的修改。

// 缺少上下文
function calculatePrice(item) {
  return item.basePrice * 0.88;
}

// 有上下文
function calculatePrice(item) {
  // 应用12%平台费用 (0.88 = 1 - 0.12)
  // 参考定价文档: https://docs.company.com/pricing
  return item.basePrice * 0.88;
}

魔法数字0.88没有上下文是不可理解的。注释解释了它的存在原因以及去哪里了解更多。未来的开发者就不必搜索整个代码库或到处打听。

资深开发者也会注意那些技术上正确但令人困惑的写法：

// 技术上能用但令人困惑
const isValid = !!data && !!data.name && data.name.length > 0;

// 相同逻辑，意图更清晰
const hasValidName = data?.name && data.name.length > 0;

第二种写法更容易理解。看一眼就能明白在检查什么，而不用解析复杂的布尔逻辑。

知道什么时候选择结对编程而不是评论

有时一个代码提交的方向本身就错了，再多的评论也改不回来。架构可能有问题、思路可能根本错误，或对问题的理解有重大偏差。那时资深开发者会建议结对编程或者实时讨论，而不是仅依赖异步代码审查。

像"我们开个会聊一下好吗？我对这个方案有些顾虑，面对面讨论比在评论里来回快得多"这样的建议能节省大量时间。作者避免白费力气去实现错误方案，审查者也不用花几个小时写长篇解释。你们可以一起解决问题，达成清晰的方案，作者在明确方向下重写代码。

代码审查评论适合针对具体、聚焦的改动。但它们并不适合用来重新思考整个方案。知道何时切换沟通方式是资深技能之一。

信任与教学

作为资深开发者在审查代码时，最困难的部分并非技术，而是与人打交道。你需要相信作者已经对他们的决策做了思考，并假设他们有理由这样做。在提出要求前先提问。

以好奇心开始——"我好奇你为什么选X而不是Y"——通常比直接说"你应该用Y"更有效。也许有你没考虑到的原因，或者他们试过Y但不可行。以好奇而非纠正开始讨论能带来更好的交流，也能让你学到东西。

同时，你也要愿意去教学。初级开发者不知道他们所不知道的东西。当你发现会导致问题的模式时，解释它们；当你建议改动时，解释原因。目标不仅仅是修好当前代码提交，而是让开发者未来写出更好的代码。

这种平衡是：信任作者的能力，同时认识到他们可能缺少识别某些陷阱的经验。也就是说，要提问、清晰解释原因，并愿意通过审查批准那些和你写法不同但仍然足够好的代码。这种社交技能——在信任、教学和同理心之间找到平衡——正是资深审查者的标志。

总结

最后我想说：代码审查并不是追求完美。它是为了在允许代码发布的前提下抓住那些重要的问题。资深开发者明白每条评论都有成本——作者的时间、发布延迟和潜在的摩擦。

问题不是"什么能做得更好？"，因为总有改进的空间。真正的问题是"在发布之前什么必须不同？"这个清单要短得多。

其他一切都可以作为下次的反馈、未来开发者的文档，或只是个人偏好而已。专注于那些会导致生产问题或使代码难以维护的问题。其他的就放手吧。

最好的代码审查能发现那些微妙的竞争条件或架构问题，这些问题本来会带来数周的痛苦。

知道二者的区别。审查真正重要的东西。