前端 Code Review 常见问题 点击关注公众号，“技术干货”及时达！ 规范类问题无意义的 commit message建议所有同学了解一下看看 commit message 规范，并且为项目配置 commit lint，感受一下两个项目 无用和缺省的依赖使用 depcheck 感受下有没有触目惊心 Unused dependencies * @alife/bc-big-brother * @alife/scc-gangesweb-inquiry * babel-runtime * caniuse-lite Unused devDependencies * @ali/kyle-config-saga * style-loader Missing dependencies * @src/utils: ./src/utils/do-dot.js * @alife/bc-safe-log: ./src/utils/log.js * @src/pages: ./src/pages/template/showroom-list/index.jsx * @src/modules: ./src/pages/template/showroom-gallery/index-v2.jsx * @src/components: ./src/pages/template/promotion-venue/index.jsx * @alife/bc-video-player: ./src/pages/template/pla-detail/product-detail.jsx dependencies 和 devDependencies 不分因为 tnpm 打平了依赖，混写虽然最终都能 work，但把 react 依赖写在 devDependencies 等行为依旧很业余，如果确实不是项目直接依赖，可以看看 peerDependencies 的使用 包引用顺序前端代码中用 import 一般有三种情况 css 代码项目内文件三方包虽然没有明确规定的顺序，但最好三者分类引用，业界也有非常多可以自动调整的工具，可以配合 eslint、prettier 等工具使用 甚至 package.json 字段的顺序也可以自动调整呢 sort-package-json 方法名不是动词短语这是个很基础的约定，看到 productListFetch()这样的方法名会很奇怪，不如 fetchProductList 遗留的 console 调试代码eslint 还是要配置一下，最好级别不是 warning，而是 error，确实有必要可以使用 eslint-disable 不合理的 eslint-disable 范围上面提到 eslint-disable，很多同学在滥用该能力，为了一行代码的某个特性，全文件禁用了 eslint，合理做法应该是只禁用某一行的某一个特性 //eslint-disable-next-lineno-alert alert('foo'); 不清楚 Dsiabling Rules 可以看看官方文档 https://eslint.org/docs/latest/user-guide/configuring/rules#disabling-rules 大段注释代码不删除无力吐槽，都有 git 记录了，只要好好写 commit message，想找回代码很容易，即使一定要保留，大促后就移除注释（也是很不好的处理方案），最起码加个 TODO 标识，让 IDE 提醒自己还有债没还 文件名用了 ts，实际代码和 ts 无关如果不认可 TypeScript 带来的价值，大可不必如此伪装 CSS 属性顺序大部分代码规约对 css 属性也没有限制，但我们一般会遵从如下的约定 布局、定位相关 display / position / float 等自身属性 width / height / margin / padding 等文本属性 color / font / text-decoration / text-align 等其它 cursor / border/ shadow / background 等顺便说一句，也有自动调整的工具 prettier-plugin-style-order CSS 类名规范都使用小写，众所周知 CSS 不区分大小写单词间使用font style="color:rgb(64, 64, 64);"/font 连字符，不要使用驼峰等，区块 block、元素 element、修饰符 modifier 可以参考 BEM 命名规范尽量不要用单个单词，即使自己限定了作用域，防不住别人?在 CSS Modules 中，推荐使用驼峰命名 https://github.com/css-modules/css-modules#naming ?静态内联样式functionDemo(){ return( divclassName="product-list-container" { list.map(product={ return( divstyle={{ width:'200px', height:'400px', backgroundColor:'#efefef' }} /div }) } /div } 这种图省事把静态样式内联，这样有两个弊端 缺少语义，不知道为什么设置这样的属性在 list 场景会显著增加 DOM 体积，SSR 场景影响 download 时间样式反复覆盖调整样式时候遇到这种会不会感觉头大 尽可能让样式作用的精准的作用域，不要依赖层层覆盖 .container{ margin:0auto; color:#333; &.mobile{ color:#444; } } 可以适当调整为 .container{ margin:0auto; { color:#333; } &.mobile{ color:#444; } } 前端工程问题代码不合并 master，使用 gitlab Tag 功能标识版本号推送不在发布流程（非常容易导致版本号先推送，然后再去法静态资源的顺序问题）lint 不生效代码没有配置 lint，这种没嘛好说的，给项目加上没有意识到 husky pre-commit 不生效了，可以查查 husky 最新的升级版本配置变化没有价值的 README项目的 README 不应该是脚手架生成的默认内容，不同类型的项目包含的信息可能不同，但都应该起到一个作用—— 项目需要交给其他同学，看 README 就够了，因此一般的前端项目 README 应该包含如下内容 项目说明项目简单介绍对应产品地址工程Aone、DEF 等应用地址使用框架的文档地址研发流程介绍环境安装流程前后端接口约定文档埋点介绍hosts 绑定开发、构建、发布等命令API 介绍字段介绍简单示例效果截图常见方法错误使用混用 forEach 和 map可能是 React 让大家习惯了用 map，而忽略了其语义 ?Array.prototype.map() 创建一个新数组，这个新数组由原数组中的每个元素都调用一次提供的函数后的返回值组成 ?arr.map(item={ item.name+='--'; }) option chain 不考虑兼容option chain 语法让我们避免了冗长的空指针检查，但大家习惯在赋值过程使用，却忽略了消费过程的兼容 constchildren=testObj?.elements?.items; return( { children.map(item={ span{item}/span }) } ); 多一层包裹的回调函数getList() .then(list={ console.log(list); 是不是感觉这样的代码太熟悉不过？ getList().then(console.log); await 带来的没必要串行调用await 很好用，但不是所有时候都合适，可能会带来没必要串行调用 constuser=awaitgetUser(userId); constfavorites=awaitgetFavaritesByUserId(userId); setStore(user,favorites); 各个函数之间没有前后依赖关系时候应该尽可能并行 Promise.all([ getUser(userId), getFavaritesByUserId(userId) ]).then(setStore); 不好的编程习惯根据现象编程if(indexOf('test')=0){} //榜单前3位特殊处理 if(index4){} if(ieVersion=9){ //使用Array.isArray方法 } 上面的三个语句都是根据句表象在处理逻辑，这种代码类似双重否定的 bool 值（ notDisabled）需要人多反应一次才能理解代码表达的含义，而且在后续的维护中很容易被人误改 我们应该回归现象的本质，用最直接的方式表达逻辑 if(indexOf('test')!=-1){} if(index=3){} if(typeofArray.isArray==='function'){ //使用Array.isArray方法 } 穷举方式写代码这类问题其实是对“根据现象编程”的一种延伸，有一道面试题 ?实现函数 compareVersion(v1, v2)，用例 compareVersion('1.0.0', '0.9.0') // 1 compareVersion('1.0.0', '1.0.0') // 0 compareVersion('1.0.0', '2.0.0') // -1 ?会不会写出这样的代码 functioncompareVersion(v1,v2){ constver1Arr=v1.split('.').map(Number); constver2Arr=v2.split('.').map(Number); if(ver1Arr[0]ver2Arr[0]){ return1; }elseif(ver1Arr[0]ver2Arr[0]){ return-1; }else{ if(ver1Arr[1]ver2Arr[1]){ return1; }elseif(ver1Arr[1]ver2Arr[1]){ return-1; }else{ if(ver1Arr[2]ver2Arr[2]){ return1; }elseif(ver1Arr[2]ver2Arr[2]){ return-1; }else{ return0; } } } return0; } 例子夸张了一些，不过类似的案例非常多，用代码穷举所有可能，下面的写法感觉是不是更有逻辑一点 functioncompareVersion(v1,v2){ constver1Arr=v1.split('.').map(Number); constver2Arr=v2.split('.').map(Number); for(leti=0;i3;i++){ if(ver1Arr[i]ver2Arr[i]){ return1; }elseif(ver1Arr[i]ver2Arr[i]){ return-1; } } return0; } 感受下这段代码更好的写法是什么 functiongetModalWidth(length){ if(length=10){ return285; } if(length10length=20){ return570; } if(length20length=30){ return855; } if(length30length=40){ return1140; } if(length10length=20){ return1425; } } 第一直觉是不是改成这样 functiongetModalWidth(length){ if(length=10){ return285; } if(length=20){ return570; } if(length=30){ return855; } if(length=40){ return1140; } if(length=50){ return1425; } } 仔细分析下是不是可以改成这样 functiongetModalWidth(length){ returnMath.ceil(length/10)*285; } Magic number & 常量抽离if(column7){} 每个人看到其他人代码中出现 Magic number 时候都会异常气愤，但到自己代码时候会觉得表意足够清晰，忽略数字变量的命名 除了注意了数字变量命名问题，对于全局反复使用的变量要注意抽离到公共文件中，便于后续的统一维护，很多工程脚手架甚至会初始化一个 constants文件夹，使用不同的文件存放各种常量 //同时渲染模块数 exportconstDRIVE_MODULE_AMOUNT_ONCE=3; //模块渲染失败最多重试次数 exportconstMODULE_RENDER_RETRY_TIMES=2; 函数有副作用({activeIndex1,floorArray})={ letfinalLiveList; if(activeIndex1===0){ floorArray[0].list[0].cardType=floorArray?.[0]?.cardType; finalLiveList=[...floorArray[0].list,...floorArray[1].list]; }else{ finalLiveList=[...floorArray[0].list]; } return( WaterFall activeIndex1={activeIndex1} list={finalLiveList} / } 函数应该避免修改入参引发副作用，尽可能保证函数是纯函数 如果函数的调用参数相同，则永远返回相同的结果。它不依赖于程序执行期间函数外部任何状态或数据的变化，必须只依赖于其输入参数该函数不会产生任何可观察的副作用({activeIndex1,floorArray})={ letfinalLiveList; if(activeIndex1===0){ finalLiveList=[...floorArray[0].list,...floorArray[1].list]; finalLiveList[0].cardType=floorArray?.[0]?.cardType; }else{ finalLiveList=[...floorArray[0].list]; } return( WaterFall activeIndex1={activeIndex1} list={finalLiveList} / } 不区分思路与实现的 renderCR React UI 逻辑，最害怕看到的就是这样的代码 functionDemo(){ return( div divclassName="demo1" div div/div div/div /div div div/div div/div /div /div divclassName="demo2" div { xxx( div/div div/div ) } /div div div/div div/div /div /div /div } 这种代码读起来实在是无从下手，整个 UI 的思路与实现杂糅在一起，需要了解作者的全部思路才能读懂。如果要 CR 一个解决“把大象塞进冰箱”的问题，那么我会希望首先了解到作者的整体思路是什么，可能有三步 打开冰箱门把大象放进去关上冰箱门开关冰箱门很好理解，快速看一下代码规约即可，重点放在把大象放进去的实现细节就可以了，如果又进一步做了思路的拆解，那代码读起来就更简单了 2.1 实现蚁人电影中的缩小技术2.2 技术应用到大象身上2.3 拎起大象丢进去所以对于复杂的 UI 我希望看到的写法是这样的 functionComponent1(){} functionComponent2(){} functionComponent3(){} functionComponent4(){} functionDemo(){ return( Component1 Component2/Component2 Component3 Component4/ Component4/ /Component3 /Component1 } 在处理复杂逻辑时候可以按照类似步骤处理一下 画流程图每个节点定义成一个函数把函数串联起来为每个函数做实现测试?相对于面向对象，可能大部分前端代码更适合用面向过程的写法 ?违背 Dont Repeat Yourself 原则这个感觉不用解释什么，但我们却在不停的违背，当想实现某个逻辑拿出的方案是“Copy 某段代码改改”时候，也需就已经违背了这个原则，无论什么理由，建议不要去按 Ctrl + C 养成好的编程习惯上面讲的都属于知识和技巧，真正重要的是写好代码的意愿，很难想象不注重代码健壮性、可读性的工程师，会把当下的代码珍视为自己的代表作，用来流传。日常不会有那么多大事落在头上，此时、此行代码就是我。 技术素养——技术领域平素的修养。 点击关注公众号，“技术干货”及时达！ 阅读原文