代码检视/代码审查/Code Review

目录

一、代码检视的目的

二、代码检视前的准备

三、代码检视九句箴言

四、代码检视checklist（经验检查项）

五、代码检视结果度量

六、代码质量衡量指标

七、高质量代码

---

代码检视是一种用来确认方案设计和代码实现的质量保证机制，通过这个机制我们可以对代码、测试过程和注释进行检查，以便提高代码质量，保证项目或产品的稳定性，积累开发经验等。

通过代码检视可以达到如下目的：

• 在项目早期就能够发现代码中的BUG
• 避免开发人员犯一些很常见，很普通的错误
• 识别常见的代码安全漏洞
• 检测可疑代码段以及集成到软件中的任何恶意代码
• 项目或产品的代码更容易维护
• 帮助初级开发人员学习高级开发人员的经验，达到知识共享
• 保证项目组人员的良好沟通
• 优化业务实现逻辑，总结最佳实践

被检视代码进入代码检视需要检查的条件如下：

• 代码检视人员是否理解了代码检视的概念和代码检视将做什么。如果做代码检视的人员不能理解代码检视对项目成败和代码质量的重要程度，他们的做法可能就会是应付了事。
• 代码是否已经正确的build，目的是使得代码已经不存在基本语法错误。我们总不希望高级开发人员或是主管将时间浪费在检查连编译都通不过的代码上吧。
• 代码执行时功能是否正确。代码检视人员也不负责检查代码的功能是否正确，也就是说，需要复查的代码必须由开发人员或质量人员负责该代码功能的正确性。
• 代码检视人员是否理解了代码。代码检视人员需要对该代码有一个基本的了解，其功能是什么，是哪一方面的代码，涉及到数据库还是通讯，这样才能采取针对性的检查。
• 开发人员是否对代码做了单元测试。这一点也是为了保证代码检视前一些语法和功能问题已经得到解决，代码检视人员可以将精力集中在代码的质量上。

检视人员的准备：

• 线上检视：如果是CICD流程中的检视，则直接利用CICD中的工具进行检视即可
• 线下检视：需准备好检视工具、检视结果汇总工具、问题代码修改反馈闭环工具等

1. 看见了if，就想else
2. 看见malloc，就去找free
3. 函数调用要小心，需要看看返回值
4. 看到for循环，就找边界值
5. 看见return要注意，要去前面找资源
6. 看见数组把神提，问题往往在下标
7. 不要小看字符串，长度是个大问题
8. 见到函数不要急，看看变量初始化，各种路径要小心
9. 赋值函数最危险，变量没有初始化
10. 九句句真言不孤立，相互结合显神威

以下为箴言详解

1. 看见了if，就想else。一看到if语句，就要想到else语句。如果没有else语句，就要分析是不需要，还是异常情况没有处理。此外，多分支if 判断一定要有最后的else语句，比如：

if (1 == flag)
{
    printf("flag = 1\n");
}
else if (2 == flag)
{
    printf("flag = 1\n");
}
else /* 多分支if语句必须要有这个else分支，哪怕这里面什么事情都不做 */
{
    /* do nothing */;
}

2. 看见malloc，就去找free。完整的应该是：看见malloc，先找判空，再去每个分支找free，全局指针free后要置空。malloc出来的内存，操作系统是不会帮助我们回收的，所以就需要我们主动调用free函数来释放malloc出来的内存。如果发现函数体里有调用malloc函数的地方，那么我们应该立即检查一下，例如：

STATUS fun1(UCHAR *ram)
{
    UCHAR *temp;
 
    temp = (UCHAR *)malloc(100);
    if (NULL == temp) /* 申请完内存后，要立即判断内存申请是否成功 */
    {
        printf("fun1 errro: malloc failed!\n");
        return ERROR;
    }
 
    .....
    
    if (ERROR == do_something(temp))
    {
        free(temp); /* 异常分支返回前要释放前面申请的内存 */
        return ERROR;
    }
 
    .....
 
    free(temp); /* 正常分支返回前要释放前面申请的内存 */
    return OK;
}

下面的例子是另一种情况，其申请的内存是供外面使用的，不能在函数内释放，但是，我们要顺藤摸瓜，追踪使用这片内存的函数，直到找到释放该内存的操作为止，并分析是否会发生内存泄漏。

STATUS GetMemory(char **p, int num)
{
    if (NULL == p)
    {
        return ERROR;
    }
 
    /* 申请内存供外面使用，需在外面追踪这块内存的释放情况，以免内存泄漏 */
    *p = (char *)malloc(sizeof(char) * num);
    if (NULL == *p)
    {
        return ERROR;
    }
 
    return OK;
}

3. 函数调用要小心，需要看看返回值。是否需要判断函数的返回值？这个问题要具体分析，遵循一个原则：如果不判断返回值会引起问题，那么就一定要判断，否则就无所谓了。比如pritnf函数也有返回值，但是就不需要判断。异常处理问题是代码检视问题的第一大户。举个例子:

STATUS GetMemory(char **p, int num)
{
    if (NULL == p)
    {
        return ERROR;
    }
 
    /* 申请内存供外面使用，需在外面追踪这块内存的释放情况，以免内存泄漏 */
    *p = (char *)malloc(sizeof(char) * num);
    if (NULL == *p)
    {
        return ERROR;
    }
 
    return OK;
}
 
int DoSomething(...)
{
    char *p = NULL;
 
    if (GetMemory(&p, 100) != OK) /* 这里就一定要判断返回值，才能使用变量p */
    {
        return ERROR;
    }
 
    memset(p, 0, 100);
    ...
 
    FreeMemory(p);  /* 内存使用完后，要释放 */    
    return OK;
}

4. 看到for循环，就找边界值。看到for循环，就要看看边界值是否合理。如果循环变量是数组的下标，更加需要注意。

5. 看见return要注意，要去前面找资源。这里所说的资源包括但不限于：内存、信号量、WaitList、文件句柄、socket、数据库连接等。看见return语句，尤其是函数中间的异常返回语句，就要马上折回头去看看前面有没有分配资源，前面分配的任何资源，在异常返回前需要一并释放，不释放就是错误！资源泄漏问题是代码检视问题的第二大户。

6. 看见数组把神提，问题往往在下标。 比如，我们定义了一个数组a[3]，那么我们能使用的数组元素只有a[0]、a[1]和a[2]，如果使用了a[3]，就将导致数组越界，改写不属于数组a的内存空间。数组越界的问题经常和for、while等循环一起出现。只要细心，数组越界的问题很容易发现。例如：

void funA()
{
    int a[6], i;
 
    for (i = 0; i <= 6; i++)  // 当i = 6时，数组越界
    {
        a[i] = i;
    }
 
    ...
}

7. 不要小看字符串，长度是个大问题。在检视字符串问题时，我们要绷紧几根弦：

1. 检查一下这个字符串是不是以“\0”结尾的。很多时候，程序员在代码里东拼西凑自己构造了一个字符串，但是结尾漏掉了“\0”。
2.  检查一下strcpy、strlen等函数的使用条件是否正确。如果某字符串中间可能有字符“\0”的话，比如从网络上得到的一段报文，那么就不能使用类似strcpy、strlen等str家族的函数。因为这些函数遇到“\0”就停止，无法正确完成我们预期的工作。遇到这种情况，正确的做法是，用memcpy函数代替strcpy函数。
3. 检查一下函数中定义的数组长度是否足够。如以下示例，如果name的长度大于20，那么就会出现数组type越界的问题。

ULONG GetTypeFromName(CHAR *name)
{
    CHAR type[20];
 
    if (NULL == name)
    {
        return ERROR;
    }
 
    strcpy(type, name); /* 如果name的长度大于20，那么就会出现数组type越界的问题。 */
    .....
}

8. 见到函数不要急，看看变量初始化，各种路径要小心。见到一个函数时，首先应该检视一下函数是否对必要的入参都进行了合法性检查。此外，要习惯性的看看所有的局部变量是不是在使用前都被初始化了。如果发现变量是在if，for，while，等语句中被初始化的，问题可能就来了。此时，我们要仔细检查这些变量是否能够在每种条件下都被初始化。例如：

void fun2(char *msg)
{
    char *str;
    ULONG errCode;
 
    // func1()的返回值为VOS_OK或VOS_ERR
    errCode = func1();
    if (VOS_OK == errCode)   /* 如果func1返回VOS_ERR，pStr无法被初始化 */
    {
        str = msg;
    }
 
    if (NULL == str)
    {
        return;
    }
 
    strcpy(str , "hello");         
    return;
}

9. 赋值函数最危险，变量没有初始化。除了第八句箴言里列举的情况，在C代码中，还经常将变量的指针作为参数，在被调用函数中对变量进行赋值。这种用法是标准的C语言的用法，无可厚非。但这种用法隐藏着极大的危险。如以下代码段，如果DEV_SelectCmdTemplet返回失败，则szTempletName就没有被初始化，CLI_UnInstallCmdMode就会使用没有初始化的指针，导致出错：

 
if (pSubObj->subPortNumber > 1)
{
    for (i = 0; i < pSubObj->subPortNumber; i++)
    {
        pPhyIfIns = DEV_GetIfFromIndex(pSubObj->phyIfTable[i]);
        if (NULL != pPhyIfIns)
        {
            ...
            
            /*
            * 如果DEV_SelectCmdTemplet返回失败，则szTempletName就没有被初始化
            * CLI_UnInstallCmdMode就会使用没有初始化的指针，导致出错
            */
            rc = DEV_SelectCmdTemplet(pPhyIfIns->ifType,
                                      &szTempletName);
 
            rc = CLI_UnInstallCmdMode(szTempletName,
                                      pPhyIfIns->ifName);
 
             DEV_IfDeleteIf(pSubObj->phyIfTable[i]);
        }
    }
}

• 编码规范方面检查项

  • 函数过长
  • 参数列表过长
  • 重复代码
  • 变量未使用
  • 函数未调用
  • 注释不准确、不规范
  • 代码缩进、格式、符号、结构等风格不一致

• 设计方面检查项

  • 结构体设计或抽象不合适
  • 不符合面向接口/对象编程的思想
  • 未采用合适的设计范式

• 资源（如内存、文件句柄、Socket、数据库连接等）泄漏处理方面检查项

  • 资源未释放，尤其在错误处理路径上
  • 资源释放多次
  • 经常大量创建临时资源
  • 未尽量使用局部资源(堆栈资源)

• 异常处理方面检查项

  • 函数异常返回时未正确处理异常，如错误记录到日志文件中
  • 未对数据的值和范围进行合法性校验，包括采用断言（assertion）
  • 在出错路径上是否所有的资源和内存都已经释放
  • 所有异常都得到正确的处理
  • 当调用导致错误发生时，方法的调用者应该得到一个通知

• 函数（接口/方法）方面检查项

  • 入参未做检查
  • 数据结构类型转换错误
  • 使用了未初始化的变量
  • 函数返回栈对象的指针
  • 数组操作越界
  • 值越界
  • 除零错误
  • 堆栈溢出
  • 接口定义不好，要尽量面向接口/对象编程，便于维护和重构

• 程序流程方面检查项

  • 循环结束条件不准确
  • 死循环
  • 无穷递归
  • switch表达式缺少break
  • 循环的处理，如循环变量，局部对象，循环次数等对性能的影响

• 性能方面检查项

  • 接口参数要避免内部转换
  • 并发访问时的应对策略
  • 同步方法的使用是否得当，是否过度使用
  • 递归方法中的递归次数是否合适，应该保证在合理的栈空间范围内
  • 如果调用了阻塞方法，是否考虑了保证性能的措施
  • 是否采用通用的线程池、对象池模块等cache技术以提高性能
  • 是否采用内存或硬盘缓冲机制以提高效率
  • I/O方面是否使用了合适的方法以提高性能（如减少序列化，使用buffer封装流等）

• 线程安全方面检查项

  • 代码中所有的全局变量是否是线程安全的
  • 需要被多个线程访问的对象是否线程安全，检查有无通过同步方法保护
  • 同步对象时是否存在可能的死锁或竞争，注意错误处理代码
  • 在保证线程安全的同时，要注意避免过度使用同步，导致性能降低

• 通讯方面检查项

  • 通讯是否存在长期阻塞问题
  • 发送接收的数据流是否采用缓冲机制
  • 通讯超时处理，异常处理
  • 数据传输的流量控制问题

• 安全方面检查项

  • 对命令行执行的代码，需要详细检查命令行参数
  • WEB类程序是否对访问参数进行合法性验证
  • 重要信息的保存是否选用合适的加密算法
  • 通讯时考虑是否选用安全的通讯方式

• 数据库处理方面

  • 数据库设计或SQL语句是否便于移植（注意和性能方面会存在冲突）
  • 数据库资源是否正常关闭和释放
  • 数据库访问模块是否正确封装，便于管理和提高性能
  • 是否采用合适的事务隔离级别
  • 是否采用存储过程以提高性能
  • 是否采用Prepared Statement以提高性能

• 其他

  • 日志是否正常输出和控制
  • 配置信息如何获得，是否有硬编码
  • 是否便于测试

DI: Defect Index(缺陷率) 

• DI= 致命级别的问题个数*10+严重级别的问题个数*3+一般级别的问题个数*1+提示级别的问题个数*0.1
• 缺陷密度 = DI*1000/lines

缺陷密度高于xx，认为代码不合格；缺陷密度少于yy，认为代码质量优秀

定量指标

• 代码复杂度：圈复杂度、认知复杂度
• 嵌套深度：嵌套如果太深，函数很容易出问题，可能需要重构
• 扇入扇出度量：扇入过大，多个模块依赖此模块，该模块很重，一定要稳定，否则影响范围极广；扇出过大，此模块依赖很多模块，考虑是否可以减少依赖

定性指标

• 可维护性：代码发布后持续相当长的时间内，应用程序力求花最少的精力来维护，而且应该易于识别缺陷和修复
• 可读性：代码应该做到不言自明，在阅读代码的同时，可以读懂业务。为变量，函数和类使用适当的命名。如果某段代码需要花很多时间来理解，则意味着有重构的必要，或者至少编写清晰明了的注释
• 可测试性：代码应易于测试，有助于更快发现代码缺陷
• 可调试性：代码需合理封装且易于执行调试，不需额外编写脚本来调试业务功能
• 可配置性：将可配置值保留在适当的文件，按环境分类以便在不同环境执行代码无需手动操作或者变更代码
• 可重用性：充分合理复用现有代码，同时评估如何降低代码耦合性
• 可靠性：代码需要确保包含异常处理和错误记录机制
• 可扩展性：只需对现有代码进行最小的更改即可轻松添加增强功能，或者说一个组件应易于替换为更好的组件
• 安全性：验证，授权，针对安全威胁的输入数据验证，敏感数据加密

• 安全性

  • 内存安全
  • 资源安全
  • 数学运算安全
  • 避免未定义行为
  • 防止缓存溢出

• 整洁代码

  • 代码可读性高
  • 没有坏味道
  • 常见的习惯用法
  • 熟悉的语言和标准库

• 风格统一

  • 代码规范统一
  • 命名风格统一
  • 词汇统一

• 高内聚低耦合

  • 消除重复
  • 缩小依赖范围
  • 分离变化方向，灵活使用设计模式

• 合理模块层次

  • 模块大小和复杂度
  • 接口大小
  • 模块之间共享资源