---
tags: Software Design
---

# Code Smell
Code Smell，中文譯作 **"程式碼的壞味道"**，指的是說程式碼中出現了一些 "特徵"，會容易造成程式容易產生bug、或是難以維護。

以下將介紹一些比較常見的code smells。

## Duplicated Code
重複程式碼應該是最基本、但同時也是最常發生的問題，而且所有人都知道他會造成什麼問題。
你要改一段code，需要同時去修改其他copy-paste的code，會造成維護上的困難。

重複程式碼一般被認為是相當嚴重的code smell，甚至可以稱為萬惡的根源。
許多軟體設計準則或方法都是為了控制或是減少重複程式碼而被發明出來的。

## Long Method
一個過長的函式常常會有這些問題：
* 很難閱讀、很難維護。
* 常常因為很多不同的理由在改這個函式。
* 不知道怎麼命名這個函式。

一個函式的首要準則，就是它必須要很**簡短**。接著，它必須只能做一件事 (SRP)。
如此一來，我很快就能看懂它在做什麼事。

根據研究統計，超過60行可能就代表著此函式過長。[\[1\]](#Citations)

## Long Parameter List
一個有很多參數的函式常常會有這些問題：
* 不知道現在要填的參數到底是第幾個參數，它的型態又是什麼。
* 用這個函式要費好大一番功夫，需要把所有參數都準備好才能call他。

根據研究統計，函式參數超過4個可能就代表參數過長。[\[1\]](#Citations)

## Large Class
一個過於龐大的class，除了閱讀與維護上的困難以外，可能同時也代表此class同時做了太多事情，容易產生 "[Divergent Change](#Divergent-Change)" 的問題。

根據研究統計，超過900行可能就代表class已經過於肥大。[\[1\]](#Citations)
然而，隨著專案規模大小不同，對於一個class的大小定義也可能不同。只要確定class在設計時沒有違反OO設計守則，就是一個好的class。

## Divergent Change
當一個class常常會因為不同的原因需要做修改，代表此class已經產生了Divergent Change的問題。換句話說，違反了 SRP。

## Shotgun Surgery
散彈式手術，指的是當修改某個class的功能時，需要同時修改許多其他的class。
這代表class之間的耦合度(coupling)太高了，在之後維護時，常常就會碰到改A忘記改B、改B忘記改C之類的問題。

"Shotgun Surgery" 與 "Diverget Change" 兩個意思看起來很像，但實際上代表的剛好是相反的概念。"Shotgun Surgery" 指的是一個原因需要同時修改到多個class的code，而 "Diverget Change" 則是指有多個原因需要改到同一個class的code。

## Data Clumps
當常常在多個地方看到3或4個以上相同的item聚集在一起時，就代表產生了Data Clumps的問題。這些地方可能是class fields、method parameters。

class Database {
    void connect(String ip, int port, String username, String password) { }
    void register(String ip, int port, String username, String password) { }
}

解決方式就是把這些items包在一個class裡。

class Connection {
    private String ip;
    private int port;
    private String username;
    private String password;
}

class Database {
    void connect(Connection c) { }
    void register(Connection c) { }
}

## Message Chain
當要存取某個資料，卻要透過一連串的getters才能取得時，就代表產生了 "Message Chain" 的問題。

// Message Chain
Person xavier = new Person();
Office office = xavier.getDepartment().getOffice();

// Solution
Office = xavier.getOffice();

"Message Chain" 可能會加強class之前的耦合度，容易遇上 "Shotgun Surgery" 的問題。
然而，對於龐大的專案而言，"Message Chain" 可能是無法避免的。此時只要能確保chain裡的class彼此之間都應該認識彼此，那麼就沒有問題。

## Feature Envy
指的是某個class很羨慕另一個class的功能；例如，`class A` 大量的呼叫另一個 `class B` 的method，那麼應該使用 "Extract Method" 的方式，將 `class B` 的該method移到 `class A`。

## Cyclomatic Complexity
指的是一個函式裡有過多的branches或loops。這可以藉由 "縮排" 數量來觀察，當發現一段code的縮排數量超過3個以上，就代表此邏輯可能複雜度過高。

當此smell產生時，除了增加測試與除錯的困難度以外，也可能代表程式的執行效率低落，應該重新思考演算法。

## Fat View
指的是一行程式碼太長了，導致讀code的時候常常要往右捲才能看完。
適當的斷行能夠大幅增加程式碼可讀性。

至於一行程式碼最多是幾個字元，這個是丟到開發者社群馬上就會引戰的議題。
其實這個議題已經在StackOverflow吵很久了，每個資深工程師都有自己的意見。

一般來說，一行程式碼最多只能有 "80-120" 個字元。
當發現你一行的程式碼超過這個數字，務必將它斷行，否則會造成閱讀上的困難。

## Memory Leak
大部分比較高階的語言都有提供GC (Garbage Collection) 的功能，所以不用特別去在乎哪邊分配了記憶體、哪邊需要釋放掉。

不過，仍然有些操作是GC無法幫上忙的。例如開了一個檔案、卻忘記把它關掉，會造成記憶體一直被占用在那邊。因此，寫程式時仍然需要特別留意 "Memory Leak" 的問題，不能過度倚賴GC。

某些高階的語言有所謂的 "Auto Close" 的語法，使用這種語法可以確保某變數離開了他所宣告的scope時，一定會釋放掉該物件所占用的資源。除此之外，也要善用 "try-catch-finally" 的語法，確定有任何例外被拋出時，仍然會進入 "finally" 進行釋放資源的動作。

## Redundant or Unused Code
常常在讀一些code時，會看到有某段程式碼被註解掉。千萬不要在原始碼裡留下一段被註解掉的程式碼，這會讓之後維護這段code的人感到困惑，而且容易造成原始碼髒亂。

好好利用版控軟體，例如Git，就可以放心的將程式碼刪除、而不是用註解的方式。未來可以利用版控軟體找出此份程式碼的修改記錄，可以找出曾經被刪除的程式碼。

另一點則是應該把沒有使用到的變數或是函式移除。雖然大部分的compiler在做優化時都會將沒用到的symbol移除，但是有些語言並沒有這功能。再加上保留沒有用到的變數或函式並沒有意義，也容易造成原始碼髒亂，因此應該將它們移除。

## A Block without Parentheses
某些語言 (例如: C/C++, Java) 支援單行語句不用使用大括號，但是這可能會產生無法預期的bug。

Apple iOS於2014年2月份的時候修正了一個SSL/TLS issue。[\[2\]](#Citations)

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

這個錯誤是絕對可以避免的，只要保持好習慣，在每個if statement後面加上大括號，就能清楚的表示在每個條件下所執行的程式碼區塊。

## If-Else or Switch Statements
"If-Else" 絕對是一份程式裡不可或缺的語句，然而它卻算是一種 "Code Smell"。其實這要看這個語法被用在什麼地方。若是在 "If-Else" 的block裡包含了大量的程式邏輯，那麼這肯定就是個code smell，應該使用 "Polymorphism" 的方式進行重構。

換句話說，"If-Else" 只能用在建立 delegate，實際上的程式邏輯寫在 delegate 裡。Delegate 可以是class或method，也就是實際上執行程式邏輯的元件。

## No Default Section in Switch Statement
在每個 "Switch" 或是 "If-Else" 語句，最好都要加上default區塊，才能清楚知道程式在什麼條件下會執行哪部分的程式碼，以及在非預期的狀態時的錯誤處理。

## Uninitialized Variables
每個變數應該都需要有適當的初始化動作，否則容易造成undefined的行為。

## Unresolved Warnings
現在許多IDE、Compiler都有提供基本的程式碼分析功能，在compile階段就會偵測並警告你說這段code可能會有問題。不要無視這些警告，要仔細看你的code為什麼會噴這些警告訊息。

## Multiplication Overflow
這是一個很基礎、但常常會不小心忽略的錯誤。而且出錯後，常常會不知道錯在哪。
當進行乘法運算時，一定要注意型別的最大值/最小值的問題。

int block_size = 4096;
int block_count = 524288;
long volume_size = block_size * block_count;

原本我預期會拿到 `volume_size` 值為 "2147483648" (2GB)，但實際上卻是-1，原因就是兩個 `int` 型態的變數相乘後overflow了。

## Mysterious Name
讀code就像在讀文章，不用註解也可以很清楚code在做什麼。因此class、method、variable的名稱應該要有意義、淺顯易懂。

Naming是一門藝術，沒有一個絕對好的命名，但是有可以遵守的naming convention。例如，class名稱必須是名詞，method名稱必須是動詞，這樣才能讓讀code的人了解這段程式在做什麼。

## Inconsistent Coding Standard
多人共同開發一份專案時，應該訂立coding standard。不一致的coding standard容易導致之後的維護困難，也會降低可讀性。

## Comments
一份程式碼到底要不要包含註解，相信這是一個頗有爭議性的話題。

我個人滿喜歡 "Clean Code" [\[3\]](#Citations) 這本書的作者所說的話。他說，閱讀程式碼就像是在讀文章，一份好的程式碼，不需要註解也能輕易的讀懂。除此之外，程式碼不會騙人，你寫什麼它就做什麼，但是註解可能會騙人。

有部分工程師主張一份程式碼應該包含至少40%的註解，但我個人認為這只會增加程式碼閱讀上的困難，再加上還需要維護註解。

一份好的程式碼不需要包含過多的註解，一般只有以下情境會需要註解：
* Copyright聲明，通常寫在原始碼的頂端。
* API說明文件。
* 複雜的程式邏輯，例如某段code實作了很艱澀的演算法，需要靠自然語言才能表達用意。
* 特殊的實作方式，例如workaround等等，需要特別註解為什麼要使用這種實作。

註解是不可或缺的，但是它不應該被氾濫的使用，只會徒增閱讀上的困難。換個角度來看，若是需要註解的地方卻沒有註解，也會增加未來維護上的困難。

---
##### Reference
* [Wiki - Code smell](https://en.wikipedia.org/wiki/Code_smell)

##### Citations
1. [Code Smells Detection and Visualization of Software Systems](https://ebooks.iospress.nl/DOI/10.3233/978-1-61499-484-8-1763)
2. [Apple iOS SSL/TLS Issue](https://www.imperialviolet.org/2014/02/22/applebug.html)
3. [Clean Code: A Handbook of Agile Software Craftsmanship](https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882)

*[SRP]: Single Responsibility Principle