注册
web

能把队友气死的8种屎山代码(React版)

前几天在前端技术群里聊起Code Review的事,大伙儿似乎都憋了一肚子气:


图片


图片


我觉得这份难言之隐应该要让更多人看到,就跟Henry约了个稿:


图片


于是Henry赶在周末,一边带娃,一边给我抹眼泪整理(脱敏)出了这篇小小的屎山合集,供大家品鉴。


以下是正文。


(文字大部分是Henry所写,沐洒进行了一些精简和调整)




1. 直接操作DOM


const a = document.querySelector('.a');

const scrollListener = throttle(() => {
  const currentY = window.scrollY;

  if (currentY > 100) {
    a.classList.add('show');
  } else {
    a.classList.remove('show');
  }
}, 300);

window.addEventListener('scroll', scrollListener);
return () => {
  window.removeEventListener('scroll', scrollListener);
};

上面的代码在监听scroll方法的回调函数中,直接上手修改DOM的类名。众所周知,React属于响应式编程,大部份情况都不需要直接操作DOM,具体原因参考官方文档(react.dev/learn/manip…


优化方法也很简单,充分发挥响应式编程的优点,用变量代替即可:


const [refreshStatus, setRefreshStatus] = useState('');

const scrollListener = throttle(() => {
  if (tab.getBoundingClientRect().top < topH) {
    setRefreshStatus('show');
  } else {
    setRefreshStatus('');
  }
}, 300);

return <div className={['page_refresh', refreshStatus].join(' ')}/>;

2. useEffect不指定依赖


依赖参数缺失。


useEffect(() => {
    console.log('no deps=====')
    // code...
});

这样的话,每次页面有重渲染,该useEffect都会执行,带来严重的性能问题。例如我们项目中,这个useEffect内部执行的是第一点中的内容,即每次都会绑定一个scroll事件的回调,而且页面中有实时轮询接口每隔5s刷新一次列表,用户在该页面稍加停留,就会有卡顿问题出现。解决方案很简单,根据useEffect的回调函数内容可知,如果需要在第一次渲染之后挂载一个scroll的回调函数,那么就给useEffect第二个参数传入空数组即可,参考官方文档(react.dev/reference/r…


useEffect(() => {
    // code...
}, []);

3. 硬编码


硬编码,即一些数据信息或配置信息直接写死在逻辑代码中,例如


图片


这两行代码本意是从url上拿到指定的参数的值,如果没有,会用一个固定的配置做兜底。


乍一看代码逻辑很清晰,但再想深一层,兜底值具体的含义是什么?为什么要用这两个值来兜底?写这行代码的同学可能很快可以解答,但是一段时间之后,写代码的人和提需求的人都找不到了呢?


这个示例代码还比较简单,拿对应的值去后台可以找到对应的含义,如果是写死的是枚举值,而且还没有类型定义,那代码就很难维护了。


图片


解决此类问题,要么将这些内容配置化,即写到一个config文件中,使用清晰的语义化命名变量;要么,至少在硬编码的地方写上注释,交代清楚这里需要硬编码的前因后果。



沐洒


关于硬编码问题,我在之前的一篇关于“配置管理”的文章里有详细阐述和应对方案,感兴趣的朋友可以看看《小白也能做出满分前端工程:01 配置管理



4. 放任文件长度,只着眼于当下的需求


很多同学做需求、写代码都比较少从全局考虑,只关注到当前需求如何完成。从“战术”上来说没有问题,快速完成产品的需求、快速迭代产品也是大家希望看到的。


可一旦只关注“战术实现”而忽略“战略设计”,除非做的产品是月抛型的,否则一定会遇到旧逻辑难以修改的情况。


如果再加上一个文件被多达10余人修改过的情况,那么每改一行代码都会是一场灾难,例如最近接手的一个页面:


图片


单文件高达1600多行!哪怕去除300多行的注释,和300多行的模板,剩下的逻辑代码也有1000行左右,这种代码可读性就极其糟糕,必须进行拆分。


而很常见的是,由于每一任经手人都疏于考虑全局,导致大量代码毫无模块化可言,甚至出现多个useEffect的依赖是完全相同的:


图片


这里明显还有另一个问题:滥用hooks。


从行号可以看出来确实是相同的依赖写了多个useEffect,很明显是多个同学各写各的的需求引入的这些hooks。

这代码跑肯定是能跑的,但是很可能会出现多个hooks中修改同一个变量,导致其他地方在使用的时候需要搞一些很tricky的操作来修Bug。


5.变量无初始值


在typescript的加持下,对变量的类型定义可以说是日益严格了。可是在一些变量的类型定义比较复杂的情况下,可能一个变量的字段很多、层级很复杂,此时有些同学就可能想偷个懒了,例如:


const [variable, setVariable] = useState();

// some code...
const queryData = function({
    // some logic
    setVariable({ showtrue });
};

useEffect(() => {
    queryData();
}, []);

return variable.show ?  : null;
 >

这里的问题很明显,如果queryData耗时比较长,在第一次渲染的时候,最后一行的variable.show就会报错了,因为variable的初始值是undefined。所以声明变量时,一定要根据变量的类型设置好有效默认值。


6. 三元选择符嵌套使用


网上很多人会推荐说用三元选择符代替简单的if-else,但几乎没有见过有人提到嵌套使用三元选择符的事情,如果看到如下代码,不知道各位读者会作何感想?


{condition1 === 1
    ? "数据加载中"
    : condition2
    ? "没有更多了"
    : condition3
    ? "当前没有可用房间"
    : "数据加载中"}

真的很难理解,明明只是一个简单的提示语句的判断,却需要拿出分析性能的精力去理解,多少有点得不偿失了。


这还只是一种比较简单的三元选择符的嵌套,因为当各个条件分支都为true时,就直接返回了,没有做更多的判断,如果再多做一层,都会直接把人的cpu的干爆炸了。 


替代方案: 



  1. 直接用if-else,可读性更高,以后如果要加逻辑也很方便。
  2. Early Return,也叫卫语句,这种写法能有效简化逻辑,增加可读性。

if (condition1 === 1return "数据加载中";
if (condition2) return "没有更多了";
if (condition3) return "当前没有可用房间";
return "数据加载中";

虽然不嵌套的三元选择符很简单,但是在例如jsx的模版中,仍然不建议大量使用三元选择符,因为可能会出现如下代码:


return (
    condition1 ? (
        <div className={condition2 ? cls1 : cls2}>
            {condition3 ? "111" : "222"}
            {condition4 ? (
                a : b} />
            ) : null
         prop1={condition5 ? 

    ) : (
        
            {condition6 ? children1 : children2}
        
    )
)

类似的代码在我们的项目中频繁出现,模版中大量的三元选择符导致文件内容拉得很长,很容易看着看着就不记得自己在哪个逻辑分支上了。


像这种简单的三元选择符,做成一个简单的memo变量,哪怕是在组件内直接写变量定义(例如:const clsName = condition2 ? cls1 : cls2),最终到模板的可读性也会比上述代码高。


7. 逻辑不拆分


React hooks可以很方便地帮助开发者聚合逻辑抽离成自定义hooks,千万不要把一个页面所有的useState、useEffect等全都放在一个文件中:


图片


其实从功能上可以对页面进行拆分,拆分之后这些变量的定义也就可以拆出去了。其中有一个很简单的原则就是,如果一个逻辑同时涉及到了useState和useEffect,那么就可以一并抽离出去成为一个自定义hooks。例如接口请求大家一般都是直接在业务逻辑中做:


const Comp = () => {
    const [data, setData] = useState({});
    const [loading, setLoading] = useState(false);
    
    useEffect(() => {
        setLoading(true);
        queryData()
            .then((response) => {
                setData(response);
            })
            .catch((error) => {
                console.error(error);
            })
            .finally(() => {
                setLoading(false);
            });
    });
    
    if (loading) return "loading...";
    
    return <div>{data.text}div>;
}

根据上面的原则,和数据拉取相关的内容涉及到了useState和useEffect,这整块逻辑就可以拆出去,那么最终就只剩下:


const Comp = () => {
    const { data, loading } = useQueryData();
    
    if (loading) return "loading...";
    
    return 
{data.text}
;
};

这样下来,Comp组件就变得身份清爽了。大家可以参考阿里的ahooks库,里面收集了很多前端常用的hooks,可以极大提升开发效率和减少重复代码。


8. 随意读取window对象的值


作为大型项目,很容易需要依赖别的模板挂载到window对象的内容,读取的时候需要考虑到是否有可能拿不到window对象上的内容,从而导致js报错?例如:


window.tmeXXX.a.func();

如果这个tmeXXX所在的js加载失败了,或者是某个版本中没有a这个属性或者func这个函数,那么页面就会白屏。


好啦,最近CR常出现的8种屎山代码都讲完了,你写过哪几种?你们团队的代码中又有哪些让你一口老血喷出来的不良代码呢?欢迎评论区告诉我。


作者:沐洒
来源:juejin.cn/post/7235663093748138021

0 个评论

要回复文章请先登录注册