臨近過年的日子，是個回顧的好機會。
正好前一段看到這篇翻譯介紹 讀自己以前代碼的Kata，拿來練習練習。
其實Thomas在提出Kata概念的時候，涵蓋的范圍是遠大于編碼層面的。只是由于其它類型的Kata的判定標準比較“虛”，所以不常拿來練習。
這個Kata的關注點在于刻意練習閱讀代碼，而且是自己寫的代碼，來體會代碼中各種細節(jié)的長期影響。通過主動的回顧來形成良好習慣，并增強review能力。
我自詡是個“代碼考古學家”，常常修一個bug把80%的時間用在了翻歷史記錄，來琢磨它為啥能寫成這個樣子。同時也是個熱衷于review別人代碼的人，不憚于對別人的代碼說三道四，想必很惹人厭了。
這次算是找個機會把自己的代碼擺出來，自食其果吧。

程序員讀到自己的代碼

代碼的選擇

Kata的要求是寫了超過一年，記憶有些模糊。代碼量在500至1000行。
我選擇了一個小項目，小到基本由我一個人開發(fā)。而且除了對口的產(chǎn)品經(jīng)理，也再沒其他人關心了。所以當初可以隨心所欲的搞。
然而即便這么小的項目，做完以后客戶卻要求與最初商定的需求文檔幾乎完全不一樣的功能。最終，所有的業(yè)務對象推倒重新做了一遍。所以也可以算是個典型的軟件項目吧。
基本上是用outside-in 式TDD開發(fā)，雖然當時還不是特別自覺的采用這種流程。

從中選擇了一個類來做Review。選擇時參考了《修改代碼的藝術》作者關于技術債分析的建議：一個類修改越頻繁，說明它越可能將來被改動，越可能存在技術債。
我統(tǒng)計了所有的提交記錄，選出一個提交次數(shù)最多的類，名叫WorkflowManager。和標準工作流沒什么關系，是一個實現(xiàn)簡單工作步驟流轉的類。

第一遍讀：假定這是大牛寫的代碼，記錄驚艷的地方

• 短。類只有200行，每個方法的長度都少于30行。
  雖說是個簡單的流程控制，用語言來描述還真不是一兩句話能說清的。相比起來代碼算是相當簡潔了。
• 一致。代碼格式，命名規(guī)范。常用寫法都很一致?？雌饋砗苷R。
• 可讀。方法和變量都起了有意義的名字。
• 測試覆蓋。類共有15個方法，對應的測試有19個用例。測試代碼量是實現(xiàn)代碼的兩倍。沒有專門用工具統(tǒng)計覆蓋率，按照從其它項目得到的經(jīng)驗，應該在90%以上。
• 每個測試都使用了統(tǒng)一的Given/When/Then格式。
• 每個測試驗證一個概念。對于相同方法不同使用情境的情況，分別寫對應的測試用例。
• 在測試中使用有意義的變量命名和assert message來說明測試的意圖，比如：
  //Given
  User original = new User("assignee before operation");
  workSet.assignee = original;
  //When
  manager.operate(workSet);
  //Then
  assertEquals("Not change assignee when operate work set", original, workSet.assignee);

第二遍讀：假定這是個二貨寫的代碼，記錄爛的地方

其實在看第一遍的時候就開始忍不住想這些地方應該寫的更好了。我果然是不善于發(fā)現(xiàn)優(yōu)點，專愛挑毛病。

• 單一職責。
  • 這個類叫做WorkflowManager, xxxManager這個名字暗示了職責不清。
  • 果然，通過Clean Code中學到的辦法，統(tǒng)計每個方法與成員變量間的關系。發(fā)現(xiàn)每個成員變量都只被很少幾個方法使用。說明這個類缺乏內(nèi)聚性，是多個職責的雜燴。
  • 在測試里也有體現(xiàn)，某幾個用例關注于某個方法。它們依賴共有的前置條件，驗證相似的結果?？雌饋砭拖袷荰est類里內(nèi)嵌了一個小Test類。
• 未使用的參數(shù)。說明當初是覺得“將來會用到”加上的這個參數(shù)，而不是由測試驅動產(chǎn)生的。
• 注釋。實現(xiàn)和測試代碼中都有說明意圖的注釋。應該重構代碼使注釋沒有必要出現(xiàn)。
• 系統(tǒng)時間。實現(xiàn)代碼里有最近更新日期的時間戳字段。測試中在操作完畢后，直接用當前系統(tǒng)日期與之比較。這樣做有兩個問題。一是依賴環(huán)境，另一個問題是如果測試執(zhí)行和驗證的兩個點恰好跨0點就會失敗。
• 返回null。由于依賴的對象有可能返回null， 這段代碼里相當一部分在處理這種情況，使得主線邏輯有些模糊。可悲的是，之后它又會返回null值給外面……
• 命名。有些方法名用的是類似事件的名稱，比如stateChanged，而不是一個動作。
• 異常處理。代碼里有一部分邏輯是處理配置信息異常情況的，這種情況下程序無法正常完成操作。然而處理的不夠統(tǒng)一，有些地方悄悄的返回null值，有些記錄了日志，但是沒有明確指出這是某項配置的問題，以及這個問題可能造成的后果。
• 測試中assert message很難說它是描述了應該的行為，還是失敗時候的錯誤。比如：
  assertNull("no start date after approval", workSet.startDate);
  到底是應該有這個日期還是沒有呢，
• 有些message在程序行為變化后沒有隨之修改。
• 每個test幾乎都有重復的mock權限相關數(shù)據(jù)的代碼，模糊了當前測試的焦點

上面這些嚴格來說，應該算是可以改進的地方，還稱不上是二。不過下面這段就……

public List<Item> setSelectedItems(List<Item> items, WorkSet workSet) {
    List<Item> remainItems = new ArrayList<Item>(items);
    nextInputItem:
    for (Item inputItem : items) {
        for (Item item : workSet.items) {
            if (item.key.equals(inputItem.key)) {
                item.selected = inputItem.selected;
                remainItems.remove(inputItem);
                continue nextInputItem;
            }
        }
        if (!inputItem.selected) {
            remainItems.remove(inputItem);
        }
    }
    return remainItems;
}

兩層for嵌套，不但嵌套for，里面還套if，不但if，還continue，不但continue，還從內(nèi)層continue到外層……
感覺差不多把今天我認為不好的風格全拿出來演練了一遍。瞬間有點“這是我寫出來的么？”的感覺。不過稍稍回憶一下，就想起來我當時還去專門查了查continue到外層的寫法。

你能看出來這是在干嘛么？
其實是數(shù)據(jù)庫里的workset保存有若干個項目，每個項目有選中狀態(tài)。每次頁面提交時候會返回這些項目通過頁面操作后的選中狀態(tài)。需要更新到數(shù)據(jù)庫中。
而且頁面還有可能增加原來數(shù)據(jù)庫里沒有項目，這些項目要在數(shù)據(jù)庫新建項目記錄，并且和workset關聯(lián)起來。
這個雙層循環(huán)做了兩件事

1. 對于已經(jīng)有的項目，按照頁面?zhèn)魅氲牧斜砀逻x中狀態(tài)
2. 對于沒有的項目，放在另一個列表中作為返回值，方便給下一步做創(chuàng)建項目等操作。

如果讓你來重構它，你會怎么做呢？

第三遍讀：假定這段代碼有個嚴重Bug，今天找不出來你就完了。記錄找到的Bug

雖然沒有專人QC，也完全沒有測試階段，但是因為由著我性子寫了一堆單元測試和集成測試。每次本地保存代碼的時候都會執(zhí)行一遍。因而我對項目的質量是相當自信的。
最直觀的感受是在客戶驗收演示會上。
當初開發(fā)完成后扔在一邊，拖了半年多客戶才驗收。已經(jīng)記不清具體代碼細節(jié)了。客戶在操作演示過程中，走到一步走不下去了。當時我心情毫無波動，一點也沒打開源代碼檢查的沖動。
果然，稍稍回憶了一下就發(fā)現(xiàn)不是bug，而是操作失誤。

所以一開始我認為是沒法完成這一輪的目標的。準備能找到個錯誤處理或者很特殊情況下的缺陷就交差吧。
而且讀代碼找bug真的好難，要完完全全讀懂每一個細節(jié)在做什么。這時候真巴不得當初代碼寫的更好讀一點就好了。
沒想到，真找到了一個

public void statedChanged(WorkSetState originalState, WorkSet workSet) {
    WorkPackage workPackage = workSet.workPackage;
    if (isApproved(workSet)) {
        /* ... */
    } else {
        for (WorkSet sibling : workPackage.workSets) {
            if (isPending(sibling)) break;
            workPackage.status = WorkPackageStatus.INPROGRESS;
        }
    }
    workPackageRepository.save(workPackage);
}

這段代碼的本意，是一次審批計劃（WorkPackage）中會分成多個分組（WorkSet），有任何一個分組還未有開始審批，那么整個計劃都處于未開始狀態(tài)。
然而真正實現(xiàn)的是，只要第一個分組是進行中，整個計劃就會變?yōu)檫M行中。如果第一個未開始，則整個計劃沒有開始。

相當出乎意料竟然有這么明顯的一個錯。我反思了一下在重重測試包圍之下還漏掉這個Bug的原因。

1. 有集成測試，但是沒有驗收測試。沒有一個地方把業(yè)務流程的故事集中講一遍。因而在較大范圍的測試中漏掉了這個點。
2. 單元測試中，僅僅構造了一個計劃有兩個分組的情況，沒覆蓋到所有分支。
   往深一層想，這個方法需要判定的情況比較多。所以針對它的測試用例本來就很多，顯得在這個測試類中占了很大的比例。也就是在第二遍讀時發(fā)現(xiàn)的好像是內(nèi)嵌了一個小的Test類的情況。寫的時候心里就有種喧賓奪主的感覺，不由得懷疑是不是對這個方法過度測試了。進而沒有仔細考慮來設計用例。
   其實內(nèi)心產(chǎn)生不協(xié)調的真正的原因是實現(xiàn)類違反了單一職責，而不是測試重心失衡。

感受

這次練習的收獲收獲頗大，雖然理論上自己有著對代碼質量，可維護性，以及測試用例設計等方面的各種觀點。但是這次時間膠囊式的體驗，還是帶來了完全不同的心理感受和思索。
特別是第三輪找Bug的挑戰(zhàn)。最后找到的時候是我記憶中找到Bug最開心的一次。