前端Code Review中的常见问题解析（一）

新酱爱学习
2,324 阅读8分钟

此文是在团队内做的分享，期望通过列举并解析code review中的常见问题，提升团队的代码规范水平。

前言

在我们日常工作中，从功能开发，到提交代码审核，是一个比较普遍的流程，不过再往后代码有没有review出问题、 需不需要修改，往往只有有限的两三个审核参与人知道。

为了帮助大家更好地理解规范，提升代码质量，这里收集了我们前端code review中的一些常见问题，并做一些解析。

代码冲突频繁、甚至把未测试通过的功能发布到了线上

主要原因是没有一个类似git-flow的合理git工作流，具体表现上为多人共用了同一分支开发、或者同一分支提测， 在发布时没有把测试不通过的功能分离出来，因此造成了冲突频繁、未测试通过功能发布上线这些问题。

以下是某项目组经历了一些代码提交的痛，而去强调的要求，应以此为鉴。

image.png

目前在项目中推荐的git工作流：

分支规则
命名格式：以/(斜线)隔离字段，-(横线)连接字符，例如：feature/goods-modules/wjh

master：生产主分支，需保持稳定性，一般由发布分支合并进来，不直接修改代码
release/production：发布分支
release/test：预发布环境(UAT)提测分支
test：测试环境提测分支
feature：新功能分支，规则：feature/功能名/人名，例如：feature/goods-modules/wjh（从master分支拉出来）
hotfix：线上bug修复分支，规则：hotfix/bug名/人名（从master分支拉出来）

工作流
开发完成：从feature 或 hotfix合并至test提测
测试通过:  从feature 或 hotfix合并至release/test进行预发布环境提测
预发布测试通过：从feature 或 hotfix合并至release/production发布，当本次发布确认稳定后，合入master分支

注意：release/test、test分支作为公共提测分支，永远只进不出，否则可能将测试中未通过的代码发布到线上！

如果是新项目从标品建新分支开发，请按照规范的格式（项目名/分支名），比如dp/master，有利于统一规范，以及方便配置gerrit审核权限。

值得注意的是，区别于传统的git-flow，该git工作流追求最大限度的新代码隔离，以避免多个功能提测时，部分功能未通过却不小心发布到了线上。

尽管如此，即使制定了合理的git工作流，我们仍不能完全杜绝问题，比如有人把test分支合到了自己的分支，比如在code review上没有严格把关，这些问题需要我们更多地去贯彻规范，避免生产事故。

Commit Message模糊、不知所云

image.png 上面列举了一些公司里的错误示例，提交描述不清晰，会直接导致我们无法理解这个commit做了什么事情，以后出了问题，也会增加代码审查的成本，目前推荐的commit格式为：

git commit -m "type(scope): subject"

公司里某项目已使用commitlint做了提交规范，可参考根目录的.commitlintrc.js文件：

/**
 * commitlint
 * 所有项目通过此模板统一配置，如有变动也统一更新，不在项目中单独修改
 *
 * 格式:
 * git commit -m "type(scope): subject"
 *
 * type-enum:
 * build    编译相关的修改，例如发布版本、对项目构建或者依赖的改动
 * chore    其他修改, 比如改变构建流程、或者增加依赖库、工具等
 * ci   持续集成修改
 * docs 文档修改
 * feat 新特性、新功能
 * fix  修复bug
 * perf 优化相关，比如提升性能、体验
 * refactor 代码重构
 * revert   回滚到上一个版本
 * style    代码格式修改, 比如空格、符号，不是css修改
 * test 测试用例修改
 *
 * 使用示例:
 * git commit -m "feat(商品管理): 新增了虚拟商品功能"
 * git commit -m "fix(埋点): 修复了用户注册的上报字段错误"
 *
 * 详细配置请参考: https://github.com/conventional-changelog/commitlint/blob/master/@commitlint/config-conventional/index.js
 */
 
module.exports = {
  extends: ['@commitlint/config-conventional'],
  rules: {
    'scope-empty': [2, 'never'],
  },
};

主要的原则是：描述清楚我们的提交是什么性质的，在哪里做了什么事情。

公共常量、方法、正则等未抽离出页面

image.png

image.png

截图3.png

能够复用的常量、方法、正则、组件等应该抽离到公共的地方统一维护，否则会生产以下一些问题：

  1. 代码冗余，经常出现重复的代码；
  2. 不利于后续修改，比如后续改了一处的常量、正则，又忘了改另一处的，导致bug的出现；
  3. 产生魔法数字，别人接手后，无法理解数字的意图，甚至连代码作者本人都忘了，只能猜。

针对上面三个图里的问题：

常量

  1. 仅在当前页面使用的常量，可在当前页面定义后再使用，必要时增加注释；
  2. 公共常量请统一在src/constants定义后使用，如果是一组常量，按照枚举的形式来定义，例如：
// 营销类型
export const MARKETING_MODE = {
  SMS: 1,
  MMS: 2,
};
 
export const MARKETING_MODE_LABELS = {
  [MARKETING_MODE.SMS]: '短信',
  [MARKETING_MODE.MMS]: '彩信',
};
 
export const MARKETING_MODE_OPTIONS = Object.values(MARKETING_MODE).map(val => ({
  value: val,
  label: MARKETING_MODE_LABELS[val],
}));

第一组可用于类型判断，第二组可用于中文字段回显，第三组可用于下拉组件选择。

命名格式上遵循全大写加下划线格式，如果是提供给第三方组件使用的，可按照第三方组件的格式，例如上面的第三组是提供给element ui使用的，label、value可以不用大写。

方法、正则、组件等

公共工具方法请放到src/utils里面，比如上面计算导航栏高度的方法放到src/utils/screen里面，正则放到src/utils/regexps里面，公共组件放到src/components里面。

如果某些情况下拿不定主意，比如涉及到原有公共模块的修改，可咨询原来的代码作者、前端组长，我们有必要为了代码的高质量而进一步探索，而不是优先考虑同时维护几份类似的代码。

文件、组件等命名格式不规范

截图 1.png

截图2 1.png

市面上常用的命名规范：

  • camelCase（小驼峰式命名法 ——首字母小写）
  • PascalCase（大驼峰式命名法 ——首字母大写）
  • kebab-case（短横线连接式）
  • snake_case（下划线连接式）

对于目录名、文件命名（包括HTML、CSS、JS、图片等等）遵循短横线连接（kebab-case）。

对于JSX组件，遵循大驼峰式命名法（PascalCase），包括在其他组件中使用时。

import Bar from './Bar';
// do something
<Bar></Bar>

对于template组件，遵循PascalCase或者kebab-case命名（但在项目中需要统一）。

对于HTML属性，遵循kebab-case命名，class也可遵循BEM规范命名。

在命名规范上其实还有很多的细则，这里只列举一部分常见问题，后续会统一整理更详细的命名规范。

变量、方法名等命名含义不准确

截图 2.png

截图2 2.png

图一里面如果我们屏蔽“播放”两个字，应该没人能够看出来eventVideo方法是用来做什么的，这里可以修改为playVideo，再按照我们事件处理加handle命名的惯例，正确的命名应该为handlePlayVideo。

图二的本意是要调用以下的人民币分和元互转方法

/**
   * 页面和服务端，汇率为100的 互相转换
   * @param money
   * @param isFromServer
   */
  convertHundredMoneyUnit(money, isFromServer) {
    if (isFromServer) {
      return !!money ? money / 100 : 0;
    }
    return !!money ? floatNum.floatMul(money, 100) : 0;
  }

其实这上面methods→moneyUtil不止有三个问题，就连这个方法本身的存在必要性、命名都是有问题的，不过这里主要讲下参数命名也需要定义清晰，能使人一目了然。

这里面val、whether可以统一为money、isFromServer，但isFromServer仍然不是最优解，需要额外的去联想到后端返回来的数据是分，才能准确了解方法的意图，所以应该改为isCent（是否为分）。

总的说来，对于命名规范，我们应该首要遵循“代码即注释”。

使用了宽松的比较（==）而不是严格比较（===）

截图 3.png 有些前端同学养成了使用==宽松比较的习惯，有时候在数据类型不统一的情况下确实是个好办法，比如前端有一个id = 5，而后端返回了一个id = '5'，

此时5 == '5'是成立的，然而也是因为这一隐式转换的特性，会埋下bug的隐患，比如：

let num = 0;
let str = "0";
let obj = new String("0");
 
console.log(num == obj); // true
console.log(num == str); // true
console.log(obj == str); // true
console.log(null == undefined); // true
console.log('' == false);  // true
console.log(0 == false);  // true

隐式转换造成的诸多可能性，会带来一些不容易发现的bug，所以推荐都使用严格比较，如果有类型不一致的情况，可以显式转换，比如id === Number(id)。

代码嵌套过深，阅读困难

Untitled 1.png 上图的代码存在层层嵌套的现象，而好的代码应该是尽量平级的，这样会有利于阅读，并且减少潜在bug的可能性。

这里面有些嵌套是不必要的，比如validate、API请求可以使用async/await，if可以使用“卫语句”，即if(!valid) return false，从而减少代码的嵌套。

更复杂的情况，我们可以考虑函数拆分、策略模式等，一个核心的思想是开放封闭原则——对扩展是开放的，而对修改是封闭的。优先通过扩展实现，而不是通过修改来实现。

样式未加scoped、嵌套层级过深等

Untitled 2.png

Untitled 3.png

Untitled 4.png

  1. PC端写样式时需要加上scoped；
  2. CSS class嵌套不超过三级；
  3. 修改element ui样式时，需要考虑统一性、可维护性，避免出现视觉不统一，以及后续有换肤等需求时，便于调整。
avatar
文章
阅读
粉丝