烂代码重构

今天接手了前同事的一个项目。一个简单的应用，竟然写了2100多行代码，40个action方法，最长的一个action有130多行。

整理前的代码缩略图

这图的底部曲线直接就反应了我看到这些代码的心情，波涛汹涌，又如滔滔江水，连绵不绝。

如果可以，我不想接手这样的烂代码。它具备了以下特征：

• 函数过长
• if else 多层嵌套
• 大量重复性代码
• 命名不规范、混乱
• 大量无用代码(接口不用了，代码不删)
• 无用注释

代码片段示例

    public function actionKidinfo() {
        $userInfo = $this->userinfo; //获取身份
        $status = array();
        $resourceId = $this->request->get('resource_id'); //书本资源ID
        if (is_object($userInfo) && $userInfo->userId !== NULL) {
            $returnUser['profile']['type'] = $userInfo->role;
            $returnUser['profile']['_id'] = $userInfo->userId;
            $returnUser['profile']['display_name'] = $userInfo->name;
            $childrenInfo = isset($this->user['children']) ? $this->user['children'] : [];

            $data = [];
            foreach ($childrenInfo as $key => $value) {
                $childData = SsoHelper::getUserInfoByIds($value);
                if ($childData['ret'] == 0 && isset($childData['data'][$value])) {
                    $returnUser['profile'][$key] = $childData;
                    $result['profile']['children'][$key]['kid_id'] = $value;
                    $result['profile']['children'][$key]['name'] = $returnUser['profile'][$key]['data'][$value]['display_name'];
                    $result['profile']['children'][$key]['thumb'] = $returnUser['profile'][$key]['data'][$value]['avatar'];
                    $result['profile']['children'][$key]['status'] = $this->recommend->Kidresourceid($value, $resourceId);
                    if ($result['profile']['children'][$key]['status'] == 'true') {
                        $status[$key] = '1';
                    }
                }
            }
            if (array_sum($status) == count($childrenInfo)) {
                $status = 1;
            } else {
                $status = 0;
            } // 给到是否可以发送推荐 关键字
            if (!empty($result['profile']['children'])) {
                $return = ['kidsinfo' => $result['profile']['children'], 'status' => $status];
                return $this->returnData(OKCODE::ok, '', $return);
            } else {
                return $this->returnData(OKCODE::no_records_found, '没有相关的信息', ['kidsinfo' => [], 'status' => $status]);
            }
        } else {
            return $this->returnData(OKCODE::no_records_found, '没有相关的信息', ['kidsinfo' => [], 'status' => $status]);
        }
    }

为了不让自己踩坑，我花了约20分钟（主要时间花在理解代码上）进行重构，并调试完毕的最后代码如下：

    public function actionKidinfo() {
        $userInfo = new UserInfo;
        if (!is_object($userInfo) || $userInfo->userId === NULL) {
            return $this->returnData(OKCODE::no_records_found, '没有相关的信息', 
              ['kidsinfo' => [], 'status' => 0]);
        }
        $resourceId = Yii::$app->request->get('resource_id');
        $children = UserHelper::getChildrenBy($userInfo->userId);
        $isAllChildrenRecommend = 1;
        foreach ($children as $key => $child) {
            $recommendStatus = RecommendModel::isResourceRecommendToKid($child['id'], $resourceId);
            $children[$key]['status'] = $recommendStatus;
            if (!$recommendStatus) {
                $isAllChildrenRecommend = 0;
            }
        }
        return $this->returnData(OKCODE::ok, '', 
          ['kidsinfo' => $children, 'status' => $isAllChildrenRecommend]);
    }

下面来说下我对付烂代码的一点心得。

1、删，快刀斩乱麻

在前端同事的配合下，我首先找出了那些已经废弃了的接口，40个接口，有用的竟然只有10个。一顿删除后，整个类只剩下800多行代码了。删完后瞬间心情好转。

而在【代码片段示例】途中中，也存在了无用的代码。

$returnUser['profile']['type'] = $userInfo->role;
$returnUser['profile']['_id'] = $userInfo->userId;
$returnUser['profile']['display_name'] = $userInfo->name;

这段代码就是各位新手都十分喜欢的操作（复制粘贴）而留下的代码隐患，在整个代码中，变量：type、_id、display_name压根是没有用到过的。

程序员信条：复制粘贴一时爽，一直复一直爽！

对于没有用的代码千万不要留，果断点删。

2、减少if else 嵌套，尽早返回

$userInfo = $this->userinfo; //获取身份
$status = array();
$resourceId = $this->request->get('resource_id'); //书本资源ID
if (is_object($userInfo) && $userInfo->userId !== NULL) {
} else{
    return $this->returnData(OKCODE::no_records_found, '没有相关的信息', ['kidsinfo' => [], 'status' => 0]);
}

改为

$userInfo = new UserInfo;
if (!is_object($userInfo) || $userInfo->userId === NULL) {
    return $this->returnData(OKCODE::no_records_found, '没有相关的信息', 
        ['kidsinfo' => [], 'status' => 0]);
}

3、提炼专职的函数

在【代码片段示例】中，有获取孩子列表的功能，而这个功能在其他地方也用到。为了更好地复用，我提炼了一个getChildrenBy($userId)的函数

    public static function getChildrenBy($userId) {
        $response = SsoHelper::getUserInfoByIds($userId);
        if (isset($response['ret']) && $response['ret'] == 0
            && isset($response['data'][$userId])) {
            $userData = $response['data'][$userId];
        } else {
            $userData = [];
        }
        $children = isset($userData['children']) ? $userData['children'] : [];
        $childrenData = SsoHelper::getUserInfoByIds($children);
        if ($childrenData['ret'] == 0) {
            $result = [];
            foreach ($childrenData['data'] as $childId => $childData) {
                $result[] = [
                    'id' => $childId,
                    'name' => $childData['display_name'],
                    'avatar' => $childData['avatar'],
                    'kid_id' => $childId,
                    'thumb' => $childData['avatar'],
                ];
            }
            return $result;
        } else {
            return [];
        }
    }

4、根据情景对进行有意义的命名

在【代码片段示例】中

$result['profile']['children'][$key]['status'] = $this->recommend->Kidresourceid($value, $resourceId);

这个Kidresourceid()是什么？第一眼看去，就要奔溃了。通读下来，我才知道原来是这个资源是否推荐给了孩子。那么按照最简单的说法，我改成了

$result['profile']['children'][$key]['status'] 
= $this->recommend->isResourceRecommendToKid($value, $resourceId);

还有其他的变量有混淆，我就不再列举了。

5、删掉无用注释

$userInfo = $this->userinfo; //获取身份
$resourceId = $this->request->get('resource_id'); //书本资源ID
            if (array_sum($status) == count($childrenInfo)) {
                $status = 1;
            } else {
                $status = 0;
            } // 给到是否可以发送推荐 关键字

虽然大家都推荐多写注释，但是这些无用的注释就不用写了，写了还给我造成混乱 “// 给到是否可以发送推荐 关键字” 通读下来这个压根就不是这个意思，如果是在需求紧急的情况下，这简直令人发狂。

在一天内不断重复这几个步骤后，代码整体的缩略图如下：

整理后的代码缩略图

看到这张图，心情总算平静了许多。

其实重构完后，整个人特别累。这次里面还是相当感谢前端同事的配合，没有配合，很难实现重构，因为那30个接口就会花费更多的时间。而且这次重构的是不涉及到其他关联应用的模块，否则花费的时间将更加多。

题外话：对于写这些烂代码的这位同事，当时我就已经明确给她指出，同时也向上司反应，但无奈她还是屡改不改。最后呢，因为程序bug太多，严重影响项目进度，被劝退了。