前端代码评审在看什么：不是挑格式，而是提前拦住后续成本

组里新来的一位实习生这周提了一个 PR，改的是订单列表页的筛选和分页功能。这类页面我们团队一年要改十几次，评审起来本该很快，但这次我在电脑前坐了将近四十分钟，前前后后留了十几条评论。倒不是代码写得差，是这段代码里几乎能踩中评审时最容易被忽略的几个坑：命名含糊、极端输入没处理、可读性差、还有一个不容易一眼看出来的性能隐患。挑这几条出来讲清楚，比讲十条评审套话更有用。

命名不是审美问题，是省下来的沟通成本

这位同事的改动里有这样一段：

复制
1// 原来的写法
2function handle(data, type) {
3  if (type === 1) {
4    return data.filter(item => item.status === 'active')
5  } else if (type === 2) {
6    return data.filter(item => item.status === 'closed')
7  }
8  return data
9}

我第一条评论就是从 handle 这个函数名开始的。它处理什么、传入的 type 是 1 还是 2 分别代表什么，函数签名完全没告诉你，得点进去读实现才知道。而 type 这种魔法数字更麻烦——半年后哪怕是这位同事自己回来看这段代码，也得回想一下 1 到底对应哪个状态。我把评论写成这样：

"这里 type 用的是 1/2 这种数字枚举，读的人得记住对照表。能不能改成有含义的字符串常量，比如 'active'/'closed'，或者干脆用一个枚举对象？函数名 handle 也太泛，看不出来它是在做订单状态过滤。"

这位同事后来改成了这样：

复制
1const ORDER_FILTER = {
2  ACTIVE: 'active',
3  CLOSED: 'closed',
4}
5
6function filterOrdersByStatus(data, filterType) {
7  if (filterType === ORDER_FILTER.ACTIVE) {
8    return data.filter(item => item.status === 'active')
9  }
10  if (filterType === ORDER_FILTER.CLOSED) {
11    return data.filter(item => item.status === 'closed')
12  }
13  return data
14}

命名这件事在评审里经常被当成"个人喜好"打太极，很多人怕显得挑剔就不提。但命名本质上是一种成本转移——你在写的时候多花十秒钟想一个准确的词，能替后面每一个读这段代码的人省下几分钟的猜测时间。尤其是 type、data、flag、handle 这几个词，几乎是我评审时看到就会追问一句"这个具体指什么"的高频词，倒不是针对哪个人，是这几个词本身信息量太低，谁写出来我都会问。

边界条件：正常路径之外的那一半代码

命名改完之后，我盯着这段分页逻辑看了一会儿：

复制
1function getPageInfo(total, pageSize, current) {
2  const totalPages = Math.ceil(total / pageSize)
3  const hasNext = current < totalPages
4  const hasPrev = current > 1
5  return { totalPages, hasNext, hasPrev }
6}

单看这几行没什么问题，跑起来也对。但我问了这位同事一句："如果 pageSize 传 0 呢？如果 total 是 0，也就是列表本身是空的呢？"

他愣了一下去试，pageSize 为 0 时 Math.ceil(total / 0) 会得到 Infinity，hasNext 永远是 true，页面上"下一页"按钮会一直亮着点不动；total 为 0 时 totalPages 变成 0，current 从 1 起步，hasPrev 判断也没问题，但如果调用方没做空列表的兜底渲染，页面上就是一个光秃秃的分页条挂在一个空表格下面，样子很怪。

这类问题在评审时容易被放过，因为它们不出现在"正常点一遍"的路径里。我自己养成的一个习惯是，看到任何做除法、做数组下标、做数量计算的函数，都会下意识地把 0、负数、空数组这几个值在脑子里代入跑一遍。这不是什么高深技巧，纯粹是踩过几次坑之后攒下的条件反射。QA 测这类页面时也总会第一时间去点空数据和极端分页，她后来跟我说过一句话我印象很深："你们开发本地测的时候数据库里永远有几十条测试数据，可线上真实场景里，新开的店铺、刚建的活动页，第一眼看到的往往就是空列表。"

这次评审我让这位同事把这几种异常输入补全：

复制
1function getPageInfo(total, pageSize, current) {
2  if (!pageSize || pageSize <= 0) {
3    return { totalPages: 0, hasNext: false, hasPrev: false }
4  }
5  const totalPages = Math.max(Math.ceil(total / pageSize), 0)
6  const hasNext = current < totalPages
7  const hasPrev = current > 1 && totalPages > 0
8  return { totalPages, hasNext, hasPrev }
9}

顺带把空列表的占位渲染也在评审意见里点了一下，让他去确认表格组件是不是已经有现成的空状态处理，不用自己再画一个。

可读性：把嵌套判断拆成能一眼读完的顺序

这段 PR 里还有一处，是筛选条件的组装逻辑：

复制
1function buildParams(form) {
2  let params = {}
3  if (form.keyword) {
4    if (form.keyword.trim()) {
5      params.keyword = form.keyword.trim()
6      if (form.searchType === 'exact') {
7        params.exact = true
8      }
9    }
10  }
11  if (form.dateRange && form.dateRange.length === 2) {
12    params.startDate = form.dateRange[0]
13    params.endDate = form.dateRange[1]
14  }
15  return params
16}

功能是对的，但三层嵌套的 if 读起来很费劲，尤其是最内层那个 exact 判断，读到那里已经得往回倒两层才知道自己在处理什么条件。我给的建议是用提前返回和逻辑运算符压平嵌套，同时用可选链把这次 PR 里另一处 form.dateRange && form.dateRange.length 这类写法也顺手统一了：

复制
1function buildParams(form) {
2  const params = {}
3  const keyword = form.keyword?.trim()
4  if (keyword) {
5    params.keyword = keyword
6    if (form.searchType === 'exact') params.exact = true
7  }
8  if (form.dateRange?.length === 2) {
9    const [startDate, endDate] = form.dateRange
10    Object.assign(params, { startDate, endDate })
11  }
12  return params
13}

可读性这件事很难量化，评审时我一般用一个笨办法——读一遍代码，中途需要往回翻看变量定义或者判断条件超过一次，就说明这段代码值得拆。嵌套超过两层的 if、超过三个参数还全是可选的函数、一行塞进三个三元表达式，都是这类信号。这不是代码洁癖，是给后面接手的人减少"读一遍记不住上下文"的负担。

性能隐患藏在看起来很朴素的循环里

前三处问题都不算特别隐蔽，真正让我多想了一会儿的是列表渲染那部分：

复制
1// 每个订单行都要展示对应的物流状态文案
2function OrderRow({ order, logisticsList }) {
3  const logistics = logisticsList.find(item => item.orderId === order.id)
4  return (
5    <tr>
6      <td>{order.id}</td>
7      <td>{logistics ? logistics.statusText : '-'}</td>
8    </tr>
9  )
10}

单独看这一行 find 没什么问题，但订单列表是循环渲染 OrderRow 的，logisticsList 又是从父组件整份传下来的原始数组。也就是说，如果一页有 50 条订单、物流列表有 200 条记录，这里就是一个 O(n×m) 的查找跑在每一次渲染里，列表稍微一大，滚动和分页切换就能感觉到卡顿。这类问题在本地开发环境几乎测不出来——测试账号下订单和物流数据都是个位数，find 一下就能出结果，问题只有在数据量上去之后才会暴露，属于典型的"跑得对，但不代表跑得快"。

我建议这位同事把物流数据在父组件里转成一个以 orderId 为键的 Map，一次转换、后面每行都是 O(1) 查找：

复制
1// 在父组件里，logisticsList 变化时才重新构建一次
2const logisticsMap = useMemo(() => {
3  const map = new Map()
4  logisticsList.forEach(item => map.set(item.orderId, item))
5  return map
6}, [logisticsList])
7
8function OrderRow({ order, logisticsMap }) {
9  const logistics = logisticsMap.get(order.id)
10  return (
11    <tr>
12      <td>{order.id}</td>
13      <td>{logistics ? logistics.statusText : '-'}</td>
14    </tr>
15  )
16}

这类隐患不是靠读单行代码就能发现的，得先在脑子里把"这个组件会渲染多少次、每次渲染要跑多重循环"过一遍。团队里一位资深同事评审时经常直接问一句"这个列表最大能到多少条"，问题不大的时候他不追究，但凡数据量有可能上到几百条，他就会要求换成 Map 或者提前建索引。这个习惯我后来也学了过来——评审看到"数组套数组"的查找，先问规模，规模小可以先放着，规模不确定或者明显会变大，就得较真。

类型层面的评审：TypeScript 能拦住一部分，但不是全部

这位同事这个项目用的是 TypeScript 4.x，团队今年从 3.x 升上来的，模板字面量类型、可变元组这些新特性还在慢慢用起来。他这次的改动里，getPageInfo 的参数原本是这样声明的：

复制
1function getPageInfo(total: number, pageSize: number, current: number) {
2  // ...
3}

类型层面确实能挡掉"传个字符串进来"这种问题，但挡不住"传个 0 进来"——0 依然是合法的 number。这也是我评审时会反复强调的一点：类型系统解决的是"类型对不对"，业务规则对不对，还是得靠人去想、去补运行时校验。我让这位同事在参数上加了一层收窄：

复制
1function getPageInfo(total: number, pageSize: number, current: number) {
2  if (pageSize <= 0) {
3    return { totalPages: 0, hasNext: false, hasPrev: false }
4  }
5  // ...
6}

评审 TypeScript 代码时还有一类常见问题是滥用 any 把类型检查直接绕过去。这次 PR 里有一处接口返回值类型没定义完整，这位同事图省事写了 res: any，我让他把接口返回结构显式声明出来：

复制
1interface OrderListResp {
2  list: OrderItem[]
3  total: number
4}
5
6async function fetchOrderList(params: OrderQuery): Promise<OrderListResp> {
7  const res = await request.get('/api/orders', { params })
8  return res.data
9}

any 不是不能用，偶尔在过渡代码、第三方类型缺失的地方用一下没问题，但如果一个 PR 里出现三五处 any，基本可以判断这次改动没有认真对待类型，评审时值得单独拎出来问一句"这里为什么不写具体类型"。

样式改动也要放进评审范围，不只是看逻辑代码

这次 PR 里筛选区域用了 flex 布局做横向排列，这位同事写的是老写法：

复制
1.filter-bar > * + * {
2  margin-left: 12px;
3}

用相邻兄弟选择器模拟间距，这是过去没有 gap 属性时候的老办法，现在 Safari 也在今年跟进支持了 flex 容器的 gap，我建议他直接换掉：

复制
1.filter-bar {
2  display: flex;
3  gap: 12px;
4}

少了一层选择器优先级的心智负担，也不用担心第一个子元素被误加上多余的左边距。评审样式改动时，我另一个常问的问题是选择器的作用范围有没有被不小心放大。这位同事这次给筛选项加了个 .item 类名，如果这个类名足够通用，很容易和页面里其他地方同名的类撞在一起。我让他确认了一下这次改动有没有用 scoped 样式或者更具体的组合选择器兜底，这类问题在 CSS 里出错代价不小——一旦样式作用域没控制住，出问题的往往不是当前页面，而是某个完全不相关的模块突然变了样子。

顺带提一句，团队现在遇到需要判断优先级的场景，已经能放心用 :is() 和 :where() 这两个今年年初随 Chrome 88 一起落地的新选择器来简化一组选择器的写法，比如把 .filter-bar .item:hover, .filter-bar .item:focus 写成 .filter-bar .item:is(:hover, :focus)，可读性好一些，也算是今年评审样式代码时新增的一个检查角度。

无障碍和语义化，容易被当成锦上添花而被评审跳过

这位同事这次的筛选表单里，输入框和标签是这样写的：

复制
1<div class="filter-item">
2  <span>关键词</span>
3  <input type="text" v-model="form.keyword" />
4</div>

功能上没问题，视觉上也对齐。但评审时我提了一句：这个 span 和 input 之间没有任何语义关联，用屏幕阅读器读这个页面，读到输入框时是读不出"关键词"这个标签的。改起来其实很简单，换成 label 并且用 for/id 关联：

复制
1<div class="filter-item">
2  <label for="keyword-input">关键词</label>
3  <input id="keyword-input" type="text" v-model="form.keyword" />
4</div>

这类问题在中后台项目里经常被认为"用户主要是内部员工，不用太讲究"，评审时也容易被跳过。但即便抛开无障碍访问不谈，label 关联 input 还带来一个很实际的交互收益——点击标签文字也能让输入框获得焦点，这对鼠标操作的普通用户同样是体验提升，不是只为了适配屏幕阅读器才做的事。我现在评审表单类的改动，都会顺手看一眼有没有用语义化标签，成本很低，但容易被写代码的人忽略。

构建层面的问题，评审时同样值得多看一眼

这次 PR 没有涉及构建配置，但团队最近处在从 webpack 4 往 5 迁移的窗口期，评审别的同事关于构建脚本的改动时，我会格外关注两类东西：一类是有没有引入不必要的全量依赖，比如为了用一个日期格式化函数把整个 moment 包引进来，评审时值得追问一句"能不能用更轻量的替代，或者只按需引入"；另一类是 webpack 5 带来的持久化缓存和 Module Federation 这些新能力，如果同事在评审里提出要试点模块联邦拆分某个子应用，我倾向于先在小范围验证，不建议直接铺到核心页面上，毕竟这一年这套方案在团队里还只是评估阶段，没有经过足够的生产验证。

评审意见怎么说，比说了什么更重要

这几条意见我没有一条是直接写"这样写不对"，而是尽量写成"这种场景下会怎样，要不要确认一下"。命名那条问的是"读的人得记住对照表吗"，极端输入那条问的是"如果传 0 会怎样"，性能那条问的是"这个列表最大能到多少条"。语气上的差别看着是小事，但决定了对方是把评审当成被挑错，还是当成一次一起把问题想清楚的讨论。

这位同事是实习生，这几条意见换成资深同事可能一句话就懂了，对他就得多铺垫一步背景，比如为什么"数字枚举"会增加理解成本、为什么"本地跑得对"不代表"极端情况也跑得对"。评审别人的代码时，评论的详细程度得跟着读的人调整，同一句话对不同经验的人信息量是不一样的。有位资深同事有次跟我聊起这个，他说他现在给新人写评审意见，习惯性会多补一句"为什么会有这个问题"，而不是只给"应该怎么改"，因为新人缺的往往不是改法，是判断标准本身。

从这次 PR 里能沉淀下来的东西

这几条意见单独看都是这一次 PR 里的具体问题，但它们背后其实是几类可以反复复用的检查点：函数和变量命名有没有传达出足够信息、看到除法和数组操作时要不要代入 0 和空值、超过两层的嵌套判断要不要拆、循环渲染里有没有藏着重复查找。这些点我们后来整理进了团队内部的 CONTRIBUTING.md，评审时如果又遇到类似问题，直接贴文档链接，比每次重新解释一遍省事很多。

命名、极端输入、可读性、性能，这四类问题几乎覆盖了前端评审里大部分真正值得花时间的地方，格式类的问题交给 ESLint 和 Prettier 去处理就够了——我们很早就把这两个工具接进了 CI 和 husky 的 pre-commit 钩子，本地提交时自动跑一遍：

复制
1# package.json
2{
3  "lint-staged": {
4    "*.{js,ts,vue}": ["eslint --fix", "prettier --write"]
5  }
6}

格式交给机器之后，评审的时间才能真正花在机器看不出来的地方——设计是不是合理、极端情况有没有漏、这段代码会不会在没人注意的场景下拖慢整个页面。

影响面比改动本身更值得关注

这位同事这次的改动只涉及一个页面，影响面还算可控。但前端代码有个特点，很多小改动看起来局部，实际影响面不一定小。前段时间有人改了公共 Button 组件的默认 type，从 'button' 改成 'submit'，理由是"大部分场景都在表单里用"，改动只有一行，评审也顺手放过了。结果上线后好几个原本只是装饰用的按钮，恰好放在 <form> 里，被点一下就触发了表单提交和页面刷新。

从那以后，遇到改公共组件、改请求拦截器、改全局样式这类改动，评审前我都会先看一眼调用方到底有多少处在用：

复制
1# 改 Button 前先看看全项目谁在用，影响面心里有个数
2grep -rn "<Button" src/ | wc -l
3grep -rn "from '@/components/Button'" src/

如果一个改动的影响面是几十上百处，那它就不是"一行代码"的评审，而是"一次约定变更"的评审。判断默认值这类东西，宁可保守一点，改默认行为永远比新增一个可选参数要危险，后者出问题的范围是新代码，前者波及的是所有存量调用。

过度封装是另一个常被放过的信号

这位同事这次的 PR 没有出现过度封装，但这是我评审时另一个会重点看的地方。前端代码里经常有人为了一个只用了一次的逻辑，硬抽一个带七八个可选参数的工具函数出来，理由是"以后好复用"。可实际情况往往是，这层抽象之后再没被第二处用过，反而让接手的人要先读懂这套抽象才能改一行业务：

复制
1// 见过的写法：为一处用法包了一层，参数还全是可选，读的人根本不知道哪些会传
2function useTable(options = {}) {
3  const { url, params, transform, immediate, pageField, sizeField /* ... */ } = options
4  // 一大坨默认值兜底
5}
6
7// 更倾向先让它直白点，等真的出现第二个场景再提炼共性
8const { data, loading } = useRequest(() => fetchUserList({ page, size }))

我评审时遇到这种情况，会直接问一句"现在有几个地方会用它"，答案常常是"就这一个"，那这层封装的意义就得打个问号。判断标准很朴素：抽象至少要有两到三个真实调用点再做，不然先把重复代码放着——重复是显性的、好改的，错误的抽象才是隐性的、难拆的。

验证方式也是评审的一部分

这位同事在 PR 描述里只写了"筛选和分页功能已实现"，我让他补了一段验证说明，包括正常筛选、清空条件、切换页码、以及前面提到的空列表和 pageSize 为 0 这几种极端情况。我们后来在 PR 模板里固定了这几项：

复制
1## 影响范围
2- [ ] 涉及页面/组件：
3## 验证
4- [ ] 正常路径：
5- [ ] 异常/极端场景（空数据、断网、超长文本、慢请求）：
6- [ ] 已跑 lint / build / 单测
7## 截图或录屏

光是"异常/极端场景"这一栏，就逼着提交者在提交前自己先想一遍空数据和慢请求的情况，很多问题在这一步就能被作者自己发现，根本不用等到评审阶段。

错误处理散落在各处，是另一个容易被放过的信号

这次 PR 里请求失败的处理是这样写的：

复制
1async function loadOrders(params) {
2  try {
3    const res = await fetchOrderList(params)
4    setList(res.list)
5  } catch (e) {
6    console.log(e)
7  }
8}

功能上看不出问题，正常路径也测得过去。但我留了一条评论："请求失败之后，页面上会发生什么？"这位同事去试了一下，答案是——什么都不会发生，列表保持上一次的数据，也没有任何提示，用户点了筛选按钮，页面像是卡住了一样，实际上是请求悄悄失败了。console.log 只在开发者工具里看得到，对用户没有任何意义。

团队这边其实早就约定了请求层统一走一层错误处理，不同页面不需要各自处理网络异常，我让这位同事改成走公共的错误提示：

复制
1async function loadOrders(params) {
2  setLoading(true)
3  try {
4    const res = await fetchOrderList(params)
5    setList(res.list)
6  } catch (e) {
7    Message.error('订单加载失败，请稍后重试')
8  } finally {
9    setLoading(false)
10  }
11}

评审请求相关代码时，我会习惯性问三件事：失败了用户能不能感知到、失败之后页面状态会不会卡在中间态、有没有绕开团队已经约定好的统一错误处理自己重新写一套。第三条尤其容易被忽略——有的同事图省事，直接在业务代码里手写一个 try/catch 弹提示，看起来解决了问题，但和项目里已有的错误提示组件、错误码映射规则不一致，时间长了整个项目的错误提示风格会变得七零八落，用户在不同页面看到的提示语气、样式都不一样。

Loading 状态漏加，是这类改动里最常见的连带问题

改错误处理的时候我顺带看了一眼 loading 状态有没有覆盖到按钮层面。筛选按钮原本是这样绑定的：

复制
1<button @click="handleSearch">查询</button>

请求发出去到返回之间这段时间，按钮是可以被重复点击的，网速一慢，用户连点两三次，就可能在短时间内发出好几个重复请求，前面提到的竞态问题在这里也会被放大。这类问题在 review 时很容易被跳过，因为它不影响"功能对不对"，只影响"体验糙不糙"，而体验问题往往要等到测试或者用户在弱网环境下反馈回来才会被注意到。我让这位同事把按钮状态和请求状态绑在一起：

复制
1<button :disabled="loading" @click="handleSearch">
2  {{ loading ? '查询中...' : '查询' }}
3</button>

这类改动改起来通常只要几行代码，但评审时得先想到要问这个问题，代码本身不会主动提醒你"这里少了个 loading"。

PR 的大小本身也值得在评审前就管控

这位同事这次的 PR 改动量不大，一个页面加几个工具函数，评审起来还算轻松。但团队里也出现过反例——有次一个同事把三个不相关的需求改动揉进同一个 PR 里提交，评审的时候我根本分不清哪一段代码是为了解决哪个问题，留言的时候得反复确认"这一行是哪个需求要的改动"。评审效率和 PR 的大小几乎是反比关系，改动越大，评审的人越容易走马观花，真正该细看的地方反而被跳过。

我们后来在提交习惯上做了个不成文的约定：一个 PR 尽量只对应一个需求或者一类修复，如果确实要顺手改点别的，也尽量拆成单独的提交，让评审的人能按提交粒度去看，而不是打开一个几十个文件变更的 diff 无从下手。这条约定不写进代码规范，但它其实决定了评审能不能认真做下去——面对一个改动范围清楚的 PR，我愿意花四十分钟像这次一样逐行推敲；面对一个混杂了好几个需求的大 PR，说实话很难保证同样的仔细程度。

弱网和时序问题，代码本身经常看不出来

评审里还有一类问题，单看代码逻辑挑不出错，得联系到运行环境才能想到。之前有个搜索框，本地开发网络很快，输入即出结果，看着很顺，但线上弱网下请求返回顺序会乱，先发的请求后到，把后面更新的结果覆盖回了旧的。这种竞态在评审时其实是能看出苗头的，关键是养成"看到异步就想一下时序"的习惯：

复制
1// 评审时会问：快速连续输入时，旧请求的结果会不会盖掉新的？
2let reqId = 0
3async function onSearch(keyword) {
4  const id = ++reqId
5  const res = await searchApi(keyword)
6  if (id !== reqId) return   // 不是最新一次请求，丢弃
7  setList(res.data)
8}

代码跑起来正不正常，和它在各种真实条件下稳不稳定，是两件事。作者往往还沉浸在"我刚写完它能跑"的状态里，容易看不全这些边角，旁边的人反而更容易冷静地问出"弱网下会怎样"这一句。

评审最终在培养的是共同的判断标准

这次给这位同事的评审意见，单条拎出来看都是具体的技术问题，但连起来看，其实是在传递团队对几件事的共识：什么样的命名算清楚、什么程度的封装算过度、哪些极端情况必须处理、哪些验证是最低要求。这些共识一旦从"某个人的口头偏好"变成团队默认的写法，后面大家写代码时自然会更接近，协作成本也会跟着往下降。

这位同事那个 PR 最后改了三版，第二版解决了命名和极端输入的问题，第三版才把性能那处 Map 优化加上。合并之后我在群里简单说了一句这几个点以后可以留意，没有铺开长篇大论——评审这件事本身已经把该讲的都讲清楚了，不需要再补一段总结陈词。真正沉淀下来的是 CONTRIBUTING.md 里新增的那几条约定，下次再遇到类似的命名或者极端输入问题，直接贴链接就是了。