爛程式碼重構

程式碼重構 · 發表 2019-03-05 20:49:57

摘要：今天接手了前同事的一個專案。一個簡單的應用，竟然寫了2100多行程式碼，40個action方法，最長的一個action有130多行。整理前的程式碼縮圖這圖的底部曲線直接就反應了我看到這些程式碼的心情，波濤洶湧，又如滔滔江水，連綿不絕。如...

今天接手了前同事的一個專案。一個簡單的應用，竟然寫了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太多，嚴重影響專案進度，被勸退了。