Code Review 总结

310 阅读8分钟

1. 前言

编程是一种工艺,刚开始编程时,最好的学习方式是能够作为学徒,参与由软件熟练工指导的正式学徒训练。

现实中,多数人都只能在不太理想的环境中训练自己,一直埋头苦干,编写代码时缺乏思考。

如何从编写可用的代码到正确的代码、甚至是优秀的代码呢?

很多开发习惯、代码细节也不知道写得好还是不好,不知道怎么提升自己的编码能力?

代码检视就是一个很好的途径,加入一个正规的团队,有 code Reviewer 和 refactor。

本文是我近期代码检视的总结。

文章目录

  • 源代码的版面和样式
  • 有意义的事物起有意义的名称
  • 样式布局
  • 性能优化

2. 源代码的版面和样式

2.1 代码组织

改前

image-20211207202806762

问题

  1. 在页面层级的目录里写一个组件,到底是想干嘛

为什么之前会这样做

  1. 没有考虑那么多,顺手就建立了。

建议

  1. 非页面组件不应该放到页面层级目录下,这样后续维护起来很混乱。要时刻思考,这是一个多人协作的项目。

2.2 引入文件规范

改前

以下代码摘自所做的 taro 小程序项目中的一小段:

import { Modal } from '../../../../components';
import { useClassnames, useStateHook, COLOR } from '../../../../utils';
import { RequiredTag } from "../../../../components/required-tag"
import edit from '../../../../images/order/edit@3x.png';
import Arrow_Icon from '../../../../images/order/arrow@2x.png';
import FRAGILE_Icon from '../../../../images/order/fragile@2x.png';

import { InsuranceEntryProps, InsuranceLineState, InsurancePackageProps, InsurancePackageState, ShowText, IInsurancePackage } from "./index.d"
import './index.scss';

export type InsuranceEntryMode = 'single' | 'multi' | ''; // 单个入口 | 保价套餐 

问题

  1. 源文件中引入的依赖顺序不规范
  2. 类型文件有本文件中声明的「InsuranceEntryMode」,也有从 「index.d.ts 」中引入。

建议

遵循一致性原则,同类型的文件放在一起,并按照以下的引入顺序:

  1. 外部 npm 包(Taro、React...)
  2. 通用组件
  3. 业务组件
  4. Services 服务
  5. utils 方法,归类分组
  6. 样式文件
  7. 静态文件,比如 icon 图片

改后

把 「InsuranceEntryMode」 加入到 index.d.ts 中

// index.d.ts
//...
export enum InsuranceEntryMode {
	single = 'single', // 单个入口
	multi = 'multi', // 保价套餐
}

组件 components 中提供 index 统一的出口,utils 归类分组。


// index.tsx
import { Modal, RequiredTag } from '../../../../components';
import { useClassnames, useStateHook } from '../../../../utils/userHooks';
import { COLOR } from '../../../../utils/constants';

import { 
  InsuranceEntryProps, 
  InsuranceLineState, 
  InsurancePackageProps, 
  InsurancePackageState, 
  ShowText, 
  InsurancePackage, 
  InsuranceEntryMode 
} from "./index.d"
import './index.scss';

import ARROW_ICON from '../../../../images/order/arrow@2x.png';
import FRAGILE_ICON from '../../../../images/order/fragile@2x.png';
import EDIT_ICON from '../../../../images/order/edit@3x.png';
```DIT_ICON from '../../../../images/order/edit@3x.png';

3. 有意义的事物起有意义的名称

3.1 变量命名

改前

const deepEqual = (object1: Object, object2: Object):boolean => {
    `const key1s = getObjectKeys(object1);` 
    `const key2s = getObjectKeys(object2);`

    if (key1s.length !== key2s.length) {
        return false;
    }

    for (const key of key1s) {
        const val1 = object1[key];
        const val2 = object2[key];
        const areObjects = isObject(val1) && isObject(val2);
        if (areObjects && !deepEqual(val1, val2) || 
            (!areObjects && val1 !== val2) 
        ) {
            return false;
        }
    }
    return true;
}

问题

这里的 key1s 和 key2s 给人的感觉就是,key1 的数组,key2 的数组。

之前为什么这样做

根本原因是没有认真去想好。

建议

key1s => keysOfObject1

key2s =>keysOfObject2

多想想,高工都是怎么命名的,他是怎样思考的。

const deepEqual = (object1: Object, object2: Object):boolean => {
    const keysOfObject1 = getObjectKeys(object1);
    const keysOfObject2 = getObjectKeys(object2);

    if (keysOfObject1.length !== keysOfObject2.length) {
        return false;
    }

    for (const key of keysOfObject1) {
        const val1 = object1[key];
        const val2 = object2[key];
        const areObjects = isObject(val1) && isObject(val2);
        if (
          	areObjects && !deepEqual(val1, val2) || 
            (!areObjects && val1 !== val2) 
        ) {
            return false;
        }
    }
    return true;
}

3.2 函数命名

改前

const renderInsurance = useMemo(() => {
    return (
        <View>
        // ....
    	</View>
    );
}, []);

问题

render 应该是一个动词,但这里返回的是一个静态的 jsx。

建议

最起码用 useCallback 表示是一个方法一个动作。

改后

const renderInsurance = useCallback(() => {
    return (
        <View>
            // ....
    	</View>
    );
}, []);

3.3 任何时候都不要使用拼音命名

改前

image.png

问题

拼音是极不推荐的名字方式,中文多音字很多,用拼音命名会造成歧义;

也会跟其他英文命名的代码有严重的不一致性。

之前为什么这样做

这些都是实际的产品分类不知怎么命名好,觉得英文会有不够准确对应的值。

建议

采用英文并不会产生歧义不够准确,可以添加上对应的注释,其他程序员如果不清楚可以直接使用翻译软件。

把所有的产品类型都放到一个 productCode 枚举类型下进行引用,如果要改,就把整个项目涉及到文件都全部改好。

改后

export interface PRODUCT_TYPE {
    STANDARD = '101', // xxx
    HEAVY = '102', // xxx
    STANDARD_LAND = '103' // xxx
}

4. 样式布局

4.1 不要以小包大

改前

image.png

问题

这里实现的效果时,点击一个小的 tag 标签,唤起一个弹框,这个弹框是铺满整个页面的。

整体的页面布局上不能以小包大。除非唤起的面板只是类似下拉框的一小部分。

之前为什么这样做

没有遵循具体的章法,只是借鉴下拉框的实现思路。

建议

把 Modal 移出去,点击 tag 时通过 showModal 这样的形式来显示 Modal。

改后

const $FragileRule = useMemo(
        () => (
            <Fragment>
                <View
                    className={classnames('package-fragile-tag-modal-header')}
                >
                    <Text
                        className={classnames(
                            'package-fragile-tag-modal-header-title'
                        )}
                    >
                        易碎保收费说明
                    </Text>
                </View>
                <View
                    className={classnames('package-fragile-tag-modal-content')}
                >
                    <View
                        className={classnames(
                            'package-fragile-tag-modal-content--1'
                        )}
                    >
                        您邮寄的物品属于易碎类!
                    </View>
                    <View
                        className={classnames(
                            'package-fragile-tag-modal-content--2'
                        )}
                    >
                        保价费用 = 物品价格 * 8%
                    </View>
                    <View
                        className={classnames(
                            'package-fragile-tag-modal-content--3'
                        )}
                    >
                        其中时效及特惠产品最低收费2元/票、快运标准达最低收费3元/票起
                    </View>
                </View>
            </Fragment>
        ),
        []
    );
    // 易碎保标签
    const $FragileTag = useMemo(() => (
        <View className={classnames('package-fragile-tag')}>
            <View
                onClick={() => {
                    showModal({
                        jsx: $FragileRule
                    })
                }}
                className={classnames('package-fragile-tag-toogle')}
            >
                <Image
                    className={classnames('package-fragile-tag-icon')}
                    src={FRAGILE_ICON}
                />
                <Text className={classnames('package-fragile-tag-text')}>
                    易碎保
                </Text>
                <Image
                    src={ARROW_ICON}
                    className={classnames('package-fragile-tag-arrow')}
                />
            </View>
        </View>
    ), []);

5. 性能优化

5.1 避免 jsx 绑定箭头函数

改前

image.png

问题

使用箭头函数会导致每次渲染都创建新的函数,就会对前一个函数进行垃圾回收,这样会对垃圾回收器造成负担。

箭头函数也会影响子组件的渲染过程,因为传入的函数 prop 每次都改变,

这样对于使用 React.memo 包裹的子组件没有起作用。

除非是遇到动态传入参数的情况,比如下面这种:

<View 
    className={classnames({
        'tab-nav-item': true,
        'tab-nav-item--active':
            activeTabKey === TAB_BAR.AVAILABLE,
    })}
    onClick={() => handleClickTabBar(TAB_BAR.AVAILABLE)}
>
    可使用({availbaleCount})
</View>

之前为什么这样做

没有考虑,觉得方便就写了。

建议

const handleClickFragileTag = useCallback(() => {
    showModal({ jsx: $FragileRule }));
}, []);

改后

const $FragileTag = useMemo(() => (
    <View 
        onClick={handleClickFragileTag} 
        className={classnames('package-fragile-tag')}
    >
        <View >
            //...
        </View>
    </View>
), []);

5.2 请求数据缓存

改前

// 全局数据
const globalData = {
    systemInfo: {},
    // 定制头部的一个信息
    customHeader: {
        contentHeight: 54,
        statusBarHeight: 20,
        height: 64
    },
    host,
    apiGateWay,
    // ..
    user: {
        authStatus: 'unAuth' // 授权状态 ---- 默认未授权
    },
    version,
    ...authConfig[process.env.NODE_ENV],
    orderCoupon: { // 下单优惠券列表
        // availbaleCount: 0,
        // availableCouponList: [],
        // unAvailableCouponList: [],
        // unAvailbaleCount: 0,
    }
}

问题

下单页面会请求优惠券接口数据,优惠券页面也会请求同一接口数据,

直接把数据缓存到 globalData 里,这是不合理的。因为优惠券的数据不属于全局数据。

这样还污染到了,需要在调用优惠券的接口写入缓存,在二次使用的页面比如优惠券页面获取缓存, 都需要加入对应的判断代码。

另外这里也不像 redux 使用统一的接口,统一更新数据源。

还有就是请求的优惠券输入参数不同,也使用到了相同的缓存。

// 下单页面写入
const [availableCouponRes, unAvailableCouponRes] = 
    await Promise.all([
        fetchAvailableCoupon(param), 
        fetchUnAvailableCoupon(param)
    ]);

globalData.orderCoupon = {
    availbaleCount,
    availableCouponList: availableCouponList || [],
    unAvailableCouponList: unAvailableCouponList || [],
    unAvailbaleCount,
}

// 优惠券页面读取
const { orderCoupon } = globalData;
if (Object.keys(orderCoupon).length > 0) {
    initCouponList(orderCoupon);
} else {
  // ...
}

建议

直接在 api 请求层面来做缓存,需要为下单可用券和下单不可用券两个接口都做缓存处理。

对输入的参数进行深比较,如果一致就进行缓存。

考虑同时两个以上的调用此 api,可用 promise 来处理,避免并发时实际上还是请求了两次数据。

之前为什么这样做

由于项目中没有使用 redux 类似的方案全局缓存数据。

疑问

需要考虑过期时间吗?优惠券的数据什么时候过期?

假如入参数据一致,但是优惠券的数据在后台已经设置,该如何处理。

就是说用户在使用小程序期间,后台数据已经更新。

回答

接口缓存的前提是,相同的输入,相同的输出。

  • 至于缓存中遇到数据的更新机率非常小,很少有人一直玩小程序获取优惠券。

  • 小程序退出时,直接清除内存就好。这种情况不需要考虑过期的问题。

改后

工具函数:

// utils.ts
interface PromiseApi {
    (...args: any): Promise<any>;
};
const promiseCacheMap = new Map();
export const withPromiseCache = async (
    promiseApi: PromiseApi,
    options: { args: any[], ignoreCompare?: boolean; }
) => {
    const {
        args = [],
        ignoreCompare = false // 是否忽略比较参数
    } = options;
    let promise: Promise<any>
    const cache = promiseCacheMap.get(promiseApi);
    if (cache) { // 有缓存
        const { args: cacheArgs, promise: cachePromise } = cache;
        if (
            ignoreCompare ||  // 忽略 args 参数依赖
            compareArray(args, cacheArgs) // args 依赖相同
        ) {
            return cachePromise;
        }
    }
    // 没有缓存记录或是没命中缓存
    promise = promiseApi(...args);
    // 记录缓存
    promiseCacheMap.set(promiseApi, { args, promise });
    return promise;
};

export const withCache = (...options) => {
    const [promiseApi, ...args] = options;
    return withPromiseCache(promiseApi, { args });
};

请求接口:

// services.ts
export const fetchAvailableCoupon = 
    async (options: IOrderCouponListOption) => 
        fetchApiJson({
            data: options,
            url: COUPON_AVAILBALE,
            errorMessage: '请求优惠券列表异常',
            errorCode: 'FETCH_COUPON_AVAILBALE_FAIL',
        });
fetchAvailableCoupon.withCache = 
    async (options: IOrderCouponListOption) => 
        withCache(fetchAvailableCoupon, options);

组件使用:

const [availableCouponRes, unAvailableCouponRes] =
    await Promise.all([
        fetchAvailableCoupon.withCache(option),
        fetchUnAvailableCoupon.withCache(option),
    ]);

上面的代码没有考虑到在后端出错的情况下对 promise 进行了删除,会出现第一个请求返回错误后(404)。

然后网络正常时,由于请求相同的参数会命中缓存,而此时的 Promise 是错误的。

那么如果只缓存成功的 Promise,失败直接重新请求,是否可行呢?

回答是不行的,因为如果第一个请求还在 pending中。这时候还没有缓存,后续的请求不会命中缓存,一样会发起多个请求的。

只能保证第一个请求发出缓存后,后续的请求再进行请求。

更好的方式是使用一个队列维护所有请求的 Promise executor,具体是:

  • 每个请求都返回一个新的Promise, Promise的exector的执行时机,通过一个队列保存。

  • 当队列长度为1的时候,执行一次请求,如果请求成功,那么遍历队列中的exector,拿到请求的结果然后resolve。

  • 如果请求失败了,那么就把这个Promise reject掉,同时出栈。然后递归调用next

  • 直到exector队列清空为止

const cacheAsync = function(promiseGenerator, cacheKey)  {
    const cache = new Map();
    const placeholder = Symbol();
    return async (params) => {
        return new Promise((resolve, reject) => {
            cacheKey = cacheKey || params;
            let cacheConfig = cache.get(cacheKey);
            if (!cacheConfig) {
                // 没有命中
                cacheConfig = {
                    hit: placeholder,
                    exector: [{ resolve, reject }]
                };
                cache.set(cacheKey, cacheConfig);
            } else {
                // 命中缓存
                // 如果已经有命中的值,直接返回
                if (cacheConfig.hit !== placeholder) {  
                    return resolve(cacheConfig.hit); 
                }
                // 把当前的 Promise resolve、reject 存入队列
                cacheConfig.exector.push({ resolve, reject }); 
            }
            
            const { exector } = cacheConfig; 

            console.log('exector', exector.length);
            // 处理并发,在请求还处于 pending 过程中就发起了相同的请求
            // 只拿第一个 Promise 作为请求入口
            if (exector.length === 1) {
                const next = async () => {
                    try {
                        if (!exector.length) return;
                        const response = await promiseGenerator(params);
                        // 如果成功了,那么直接 resolve 掉剩余同样的请求
                        while(exector.length) { // 清空
                            exector.shift().resolve(response);
                        }
                        // 缓存结果
                        cacheConfig.hit = response;
                    } catch(error) {
                        // 如果失败了,那么这个 promise 的则为 reject
                        const { reject } = exector.shift();
                        reject(error);
                        next(); // 失败重试,降级为串行
                    }
                };
                next();
            }
        })
    }
}; 

async function fetchData() {
    const res = await fetch("http://localhost:3000/test");
    const data = await res.json();
    return data;
}

const fetchData = cacheAsync2(fetchData);

测试并发 Demo:github.com/naluduo233/…

6. 小结

养成良好的开发习惯,多思考,每一次的 Code Review 都是难得的学习机会。

每一次提交 Commit 之前,自己都应该先检视一遍,多注意一些细节上的问题。

参考资料