原文鏈接:http://dreamhead.blogbus.com/logs/83450989.html
看到下面這段代碼，你會做何感想？
if(db.Next()) {
return true;
} else {
return false;
}
有的人會想，怎麼寫得這麼笨啊！但是，請放心，絕對會有人這麼想，挺好的，實現功能了。這並非我臆造出的代碼，而是從一個真實的codebase上找到。
成為一個諮詢師之後，我有機會在不同的項目中穿梭。同客戶合作的過程中，我經常乾的一件事是：code diff。也就是用源碼管理工具的diff功能把當天全部修改拿出來，從編碼的角度來分析代碼寫得怎麼樣。
因為這個工作，我看到了許多不同人編寫的代碼，我的編碼底線不斷受到挑戰。許多東西，我以為是常識，但實際上不為許多人所知，比如上面那段代碼。
我把在看到的問題總結成一個session，與客戶的程序員分享。結束之後，有人建議，為什麼不能把這些問題寫下來，與更多人分享。於是，我產生了寫下《代碼之丑》念頭，以此向《代碼之美 》致敬。
最後要說的是，開始那段代碼可以寫成這樣：
return db.Next();
諸位看官，上代碼：
if (0 == iRetCode) {
this->SendPeerMsg("000", "Process Success", outRSet);
} else {
this->SendPeerMsg("000", "Process Failure", outRSet);
}
乍一看，這段代碼還算比較簡短。那下面這段呢？
if(!strcmp(pRec->GetRecType(), PUB_RECTYPE::G_INSTALL)) {
CommDM.jkjtVPDNResOperChangGroupInfo(
const_cast(CommDM.GetProdAttrVal("vpdnIPAddress",
&(pGroupSubs->m_ProdAttr))),
true);
} else {
CommDM.jkjtVPDNResOperChangGroupInfo(
const_cast(CommDM.GetProdAttrVal("vpdnIPAddress",
&(pGroupSubs->m_ProdAttr))),
false);
}
看出來問題了嗎？經過仔細的對比，我們發現，對於如此華麗的代碼，if/else的執行語句真正的差異只在於一個參數。第一段代碼，二者的差異只是發送的消息，第二段代碼，差異在於最後那個參數。
看破這個差異之後，新的寫法就呼之欲出了，以第一段代碼為例：
const char* msg = (0 == iRetCode ? "Process Success" : "Process Failure");
this->SendPeerMsg("000", msg, outRSet);
為了節省篇幅，我選擇了條件表達式。我知道，很多人不是那麼喜歡它。如果if/else依舊是你的大愛，勇敢追求去吧！
由這段代碼調整過程，我們得出一個簡單的規則：
這裡判斷條件真正判斷的內容是消息的內容，而不是消息發送的過程。經過我們的調整，得到消息內容和和發送消息的過程嚴格分離開來。
消除了代碼中的冗餘，代碼也更容易理解，同時，給未來留出了可擴展性。如果將來iRetCode還有更多的情形，我們只要把消息獲取的時候進行調整就好了。當然，封裝成一個函數是一個更好的選擇，這樣代碼就變成了：
this->SendPeerMsg("000", peerMsgFromRetCode(iRetCode), outRSet);
至於第二段代碼的調整，留給你練手了。
這樣醜陋的代碼是如何從眾多代碼中脫穎而出的呢？很簡單，只要看到，if/else兩個執行塊裡面的內容相差無幾，需要我們人工比字元尋找差異，恭喜你，你找到它了。
這是一個長長的判斷條件：
if ( strcmp(rec.type, "PreDropGroupSubs") == 0
|| strcmp(rec.type, "StopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "QFStopUserGroupSubs") == 0
|| strcmp(rec.type, "QFStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "QZStopUserGroupSubs") == 0
|| strcmp(rec.type, "QZStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "SQStopUserGroupSubs") == 0
|| strcmp(rec.type, "SQStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "StopUseGroupSubs") == 0
|| strcmp(rec.type, "PreDropGroupSubsCancel") == 0)
之所以注意到它，因為最後兩個條件是最新修改裡面加入的，換句話說，這不是一次寫就的代碼。單就這一次而言，只改了兩行，這是可以接受的。但這是遺留代碼。每次可能只改了一兩行，通常我們會不只一次踏入這片土地。經年累月，代碼成了這個樣子。
這並非我接觸過的最長的判斷條件，這種代碼極大的開拓了我的視野。現在的我，即便面對的是一屏無法容納的條件，也可以坦然面對了，雖然顯示器越來越大。
其實，如果這個判斷條件是這個函數里僅有的東西，我也就忍了。遺憾的是，大多數情況下，這隻不過是一個更大函數中的一小段而已。
為了讓這段代碼可以接受一些，我們不妨稍做封裝：
bool shouldExecute(Record& rec) {
return (strcmp(rec.type, "PreDropGroupSubs") == 0
|| strcmp(rec.type, "StopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "QFStopUserGroupSubs") == 0
|| strcmp(rec.type, "QFStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "QZStopUserGroupSubs") == 0
|| strcmp(rec.type, "QZStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "SQStopUserGroupSubs") == 0
|| strcmp(rec.type, "SQStopUserGroupSubsCancel") == 0
|| strcmp(rec.type, "StopUseGroupSubs") == 0
|| strcmp(rec.type, "PreDropGroupSubsCancel") == 0);
}
if (shouldExecute(rec)) {
...
}
現在，雖然條件依然還是很多，但和原來龐大的函數相比，至少它已經被控制在一個相對較小的函數里了。更重要的是，通過函數名，我們終於有機會說出這段代碼判斷的是什麼了。
提取函數把這段代碼混亂的條件分離開來，它還是可以繼續改進的。比如，我們把判斷的條件進一步提取：
bool shouldExecute(Record& rec) {
static const char* execute_type[] = {
"PreDropGroupSubs",
"StopUserGroupSubsCancel",
"QFStopUserGroupSubs",
"QFStopUserGroupSubsCancel",
"QZStopUserGroupSubs",
"QZStopUserGroupSubsCancel",
"SQStopUserGroupSubs",
"SQStopUserGroupSubsCancel",
"StopUseGroupSubs",
"PreDropGroupSubsCancel"
};
int size = ARRAY_SIZE(execute_type);
for (int i = 0; i < size; i++) {
if (strcmp(rec.type, execute_type[i]) == 0) {
return true;
}
}
return false;
}
這樣的話，再加一個新的type，只要在數組中增加一個新的元素即可。如果我們有興趣的話，還可以進一步對這段代碼進行封裝，把這個type列表變成聲明式，進一步提高代碼的可讀性。
發現這種代碼很容易，只要看到在長長的判斷條件，就是它了。要限制這種代碼的存在，我們只要以設定一個簡單的規則：
在實際的應用中，我們會把“3”定義為“多”，也就是如果有兩個條件的組合，可以接受，如果是三個，還是改吧！
雖然通過不斷調整，這段代碼已經不同於之前，但它依然不是我們心目中的理想代碼。出現這種代碼，往往意味背後有更嚴重的設計問題。不過，它並不是這裡討論的內容，這裡的討論就到此為止吧！
sinojelly在《代碼之丑（二） 》的評論里問了個問題，“把這個type列表變成聲明式”，什麼樣的聲明式？
好吧！我承認，我偷懶了，為了省事，一筆帶過了。簡單理解聲明式的風格，就是把描述做什麼，而不是怎麼做。一個聲明式編程的例子是Rails裡面的數據關聯，為人熟知的has_many和belongs_to。通過聲明，模型類就會具備一些數據關聯的能力。
具體到實際開發里，聲明式編程需要有兩個部分：一方面是一些基礎的框架性代碼，另一方面是應用層面如何使用。框架代碼通常來說，都不像應用層面代碼那麼好理解，但有了這個基礎，應用代碼就會變得簡單許多。
針對之前的那段代碼，按照聲明性編程風格，我改造了代碼，下面是框架部分的代碼：
#define BEGIN_STR_PREDICATE(predicate_name) \
bool predicate_name(const char* field) { \
static const char* predicate_true_fields[] = {
#define STR_PREDICATE_ITEM(item) #item ,
#define END_STR_PREDICATE \
};\
\
int size = ARRAY_SIZE(predicate_true_fields);\
for (int i = 0; i < size; i++) { \
if (strcmp(field, predicate_true_fields[i]) == 0) {\
return true;\
}\
}\
\
return false;\
}
這裡用到了C/C++常見的宏技巧，為的就是讓應用層面的代碼寫起來更像聲明。對比一下之前的函數，就會發現，實際上二者幾乎是一樣的。有了框架，就該應用了：
BEGIN_STR_PREDICATE(shouldExecute)
STR_PREDICATE_ITEM(PreDropGroupSubs)
STR_PREDICATE_ITEM(StopUserGroupSubsCancel)
STR_PREDICATE_ITEM(QFStopUserGroupSubs)
STR_PREDICATE_ITEM(QFStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(QZStopUserGroupSubs)
STR_PREDICATE_ITEM(QZStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(SQStopUserGroupSubs)
STR_PREDICATE_ITEM(SQStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(StopUseGroupSubs)
STR_PREDICATE_ITEM(SQStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(StopUseGroupSubs)
STR_PREDICATE_ITEM(PreDropGroupSubsCancel)
END_STR_PREDICATE
shouldExecute就此重現出來了。不過，這段代碼已經不再像一個函數，而更像一段聲明，這就是我們的目標。有了這個基礎，實現一個新的函數，不過是做一段新的聲明而已。
接下來就是如何使用了，與之前略有差異的是，這裡為了更好的通用性，把字元串作為參數傳了進去，而不是原來的整個類對象。
shouldExecute(r.type);
雖然應用代碼變得簡單了，但寫出框架的結構是需要一定基礎的。它不像應用代碼那樣來得平鋪直敘，但其實也沒那麼難，只不過很多人從沒有考慮把代碼寫成這樣。只要換個角度去思考，多多練習，也就可以駕輕就熟了。
又見switch：
switch(firstChar) {
case ‘N’:
nextFirstChar = ‘O’;
break;
case ‘O’:
nextFirstChar = ‘P’;
break;
case ‘P’:
nextFirstChar = ‘Q’;
break;
case ‘Q’:
nextFirstChar = ‘R’;
break;
case ‘R’:
nextFirstChar = ‘S’;
break;
case ‘S’:
nextFirstChar = ‘T’;
break;
case ‘T’:
throw Ebusiness();
default:
}
出於多年編程養成的條件反射，我對於switch總會給予更多的關照。研習面向對象編程之後，看見switch就會想到多態，遺憾的是，這段代碼和多態沒什麼關係。仔細閱讀這段代碼，我找出了其中的規律，nextFirstChar就是firstChar的下一個字元。於是，我改寫了這段代碼：
switch(firstChar) {
case ‘N’:
case ‘O’:
case ‘P’:
case ‘Q’:
case ‘R’:
nextFirstChar = firstChar + 1;
break;
case ‘T’:
throw Ebusiness();
default:
}
現在，至少看起來，這段代碼已經比原來短了不少。當然這麼做基於一個前提，也就是這些字母編碼的順序確確實實連續的。從理論上說，開始那段代碼適用性更強。但在實際開發中，我們碰到字母不連續編碼的概率趨近於0。
但這段代碼究竟是如何產生的呢？我開始研讀上下文，原來這段代碼是用當前ID產生下一個ID的，比如當前是N0000，下一個就是N0001。如果數字滿了，就改變字母，比如當前ID是R9999，下一個就是T0000。在這裡，字母也就相當於一位數字，根據情況進行進位，所以有了這段代碼。
代碼上的註釋告訴我，字母的序列只有從N到T，根據這個提示，我再次改寫了這段代碼：
if (firstChar >= ‘N’ && firstChar <= ‘S”) {
nextFirstChar = firstChar + 1;
} else {
throw Ebusiness();
}
這裡統一處理了字母為T和default的情形，嚴格說來，這和原有代碼並不完全等價。但這是了解了需求后做出的決定，換句話說，原有代碼在這裡的處理中存在漏洞。
修改這段代碼，只是運用了非常簡單的編程技巧。遺憾的是，即便如此簡單的編程技巧，也不是所有開發人員都駕輕就熟的，很多人更習慣於“平鋪直敘”。這種直白造就了代碼中的許多鴻篇巨製。我聽過不少“編程是體力活”的抱怨，不過，能把寫程序干成體力活，也著實不值得同情。寫程序，不動腦子，不體力才怪。
無論何時何地，只要switch出現在眼前，請提高警惕，那裡多半有坑。
這是一個找茬的遊戲，下面三段代碼的差別在哪：
if ( 1 == SignRunToInsert) {
RetList->Insert(i, NewCatalog);
} else {
RetList->Add(NewCatalog);
}
if ( 1 == SignRunToInsert) {
RetList->Insert(m, NewCatalog);
} else {
RetList->Add(NewCatalog);
}
if ( 1 == SignRunToInsert) {
RetList->Insert(j, NewPrivNode);
} else {
RetList->Add(NewPrivNode);
}
答案時間：除了用到變數之外，完全相同。我想說的是，這是我從一個文件的一次diff中看到的。
不妨設想一下修改這些代碼時的情形：費盡九牛二虎之力，我終於找到該在哪改動代碼，改了。作為一個有職業操守的程序員，我知道別的地方也需要類似的修改。於是，趁人不備，把剛做修改拷貝了一份，放到另外需要修改的地方。修改了幾個變數，編譯通過了。世界應該就此清凈，至少問題解決了。
好吧！雖然這個程序員有職業操守的程序員，卻缺少了些職業技能，至少在揮舞“拷貝粘貼”時，他沒有嗅到散發出的臭味。
只要意識到壞味道，修改是件很容易的事，提出一個新函數即可：
void AddNode(List& RetList, int SignRunToInsert, int Pos, Node& Node) {
if ( 1 == SignRunToInsert) {
RetList->Insert(Pos, Node);
} else {
RetList->Add(Node);
}
}
於是，原來那三段代碼變成了三個調用：
AddNode(RetList, SignRunToInsert, i, NewCatalog);
AddNode(RetList, SignRunToInsert, m, NewCatalog);
AddNode(RetList, SignRunToInsert, j, NewPrivNode);
當然，這種修改只是一個局部的微調，如果有更多的上下文信息，我們可以做得更好。
重複，是最為常見的壞味道。上面這種重複實際上是非常容易發現的，也是很容易修改。但所有這一切的前提是，發現壞味道。
長時間生活在這種代碼裡面，我們會對壞味道失去嗅覺。更可怕的是，一個初來乍到的嗅覺尚靈敏的人意識到這個問題，那些失去嗅覺的人卻告誡他，別亂動，這挺好。
趁嗅覺尚在，請堅持代碼正義。
不知道為什麼，初見它時，我想起了郭芙蓉的排山倒海：
ColdRule *newRule = new ColdRule();
newRule->SetOID(oldRule->GetOID());
newRule->SetRegion(oldRule->GetRegion());
newRule->SetRebateRuleID(oldRule->GetRebateRuleID());
newRule->SetBeginCycle(oldRule->GetBeginCycle() + 1);
newRule->SetEndCycle(oldRule->GetEndCycle());
newRule->SetMainAcctAmount(oldRule->GetMainAcctAmount());
newRule->SetGiftAcctAmount(oldRule->GetGiftAcctAmount());
newRule->SetValidDays(0);
newRule->SetGiftAcct(oldRule->GetGiftAcct());
rules->Add(newRule);
就在我以為這一片代碼就是完成給一個變數設值的時候，突然，在那個不起眼的角落裡，這個變數得到了應用：它被加到了rules裡面。什麼叫峰迴路轉，這就是。
既然它給了我們這麼有趣的體驗，必然先殺后快。下面重構了這個函數：
ColdRule* CreateNewRule(ColdRule& oldRule) {
ColdRule *newRule = new ColdRule();
newRule->SetOID(oldRule.GetOID());
newRule->SetRegion(oldRule.GetRegion());
newRule->SetRebateRuleID(oldRule.GetRebateRuleID());
newRule->SetBeginCycle(oldRule.GetBeginCycle() + 1);
newRule->SetEndCycle(oldRule.GetEndCycle());
newRule->SetMainAcctAmount(oldRule.GetMainAcctAmount());
newRule->SetGiftAcctAmount(oldRule.GetGiftAcctAmount());
newRule->SetValidDays(0);
newRule->SetGiftAcct(oldRule.GetGiftAcct());
return newRule
}
rules->Add(CreateNewRule(*oldRule));
把這一堆設值操作提取了出來，整個函數看上去一下子就清爽了。不是因為代碼變少了，而是因為代碼按照它職責進行了劃分：創建的歸創建，加入的歸加入。之前的代碼之所以讓我不安，多重職責是罪魁禍首。
談論乾淨代碼時，我們總會說，函數應該只做一件事。函數做的事越多，就會越冗長。也就越難發現不同函數內存在的相似之處。為了一個問題，要在不同的地方改來改去也就難以避免了。
即便大家都認同了函數應該只做一件事，但多大的一件事算是一件事呢！不同的人心裡有不同的標準。有人甚至認為一個功能就是一件事。於是，代碼會越來越刺激。
想寫乾淨代碼，就別怕事小。哪怕一個函數只有一行，只要它能完整的表達一件事。在乾淨代碼的世界里，大心臟是不受喜歡的。
接下來，我需要用歷經滄桑的口吻告訴你，這麼跌宕起伏的代碼也只不過是一個更大函數的一個部分。此刻，浮現在我腦海里的是層巒疊嶂的山峰。
[火星人 ] 專欄：代碼之丑已經有675次圍觀
