編程錯題本——解析OpenWnn（3）結構問題

結構問題是所有問題中最嚴重的問題，這裡的很多問題都反映了作者缺少基本常識。

代碼的結構就像房屋的結構一樣，是支撐整個代碼的架構。如果房屋的結構不夠結實，房屋就會倒塌。如果代碼的結構不夠良好那麼修改Bug或者對應需求變更的時候就要付出昂貴的代價。

1. 缺少MVC結構

    嚴重程度：非常高

     IME和其他應用程式一樣，都是由：資料(Model)，視圖(View)和控制器(Control)構成的，但是OpenWnn沒有按照MVC劃分類包，也難以區分哪部分代碼是資料，那部分代碼是視圖。

   

2.錯誤的從屬關係

    嚴重程度：非常高

    各種對象之間應該有一定的從屬關係，例如：圖書管理有書架，書架上面有圖書。就絕不可以說書裡面有圖書館，儘管可能是一本關於圖書館的書籍。

    在OpenWnn類中，有InputViewManager對象定義，而在InputViewManager的實作類別DefaultSoftKeyboard中又有OpenWnn mWnn對象定義。這就說明，當時的代碼設計者並沒有弄清楚二者的從屬關係。

    同樣的，對於SHIFT/ALT狀態，Converter類型，Engine類型，Vibrator，Sound的設定資料混合在一起存放也都是從屬關係不明的具體表現。

3. 包結構胡亂劃分

   嚴重程度：高

   包結構胡亂劃分包括包結構劃分不正確、類歸屬包不正確等各種情況。

   而OpenWnn屬於：沒有包結構的情況。雖然其中有JAJP和EN兩個子包，但是，詞典、設定、翻譯等應該屬於不同的包。它們很難說明屬於同一個類別。

 

4.耦合度偏高

    嚴重程度：高

    耦合度的高低和Bug的多少是直接相關的，所以高內聚低耦合都是編程人員的目標。

    OpenWnn中的OpenWnnEvent是一個訊息機制。本來訊息機制是用來解耦合的，將兩個不相干卻要相互操作的對象分開。通過訊息機製作為中間人來傳遞資訊並執行。但是在OpenWnn中這個Event只是一個Event而已，它非但沒有降低耦合度，反而增加了耦合度，DefaultSoftKeyboard.mWnn對象就是為此而定義的。

5.繼承關係錯誤

    嚴重程度：非常高

    SymbolList是繼承自WnnEngine的一個類，實際上這個類和翻譯沒有任何關係。

    繼承關係錯誤會引起擴充困難。並且從命名上來說，SymbolList沒有反映出任何Engine的資訊來。

 

6.數組冗餘

   嚴重程度：高

   定義一個數組超過實際使用大小，而超過部分沒有進行任何賦值，則當訪問超範圍資料時，會發生NullPointerException。

   DefaultSoftKeyboard.mKeyboard就是這種類型的數組。它採用了6維數組來存放資料。其中倒數第二維為Max of Key Modes，定義為7，但是實際上不同的模式不都是7種鍵盤，這就會形成空儲存空間。

 

7. 冗長的分支結構

    嚴重程度：高

    非常多的分支結構說明了條件多，處理路徑多，代碼複雜度高，同時也就會產生更多的Bug。另外，冗長的分支結構導致了方法的長度偏長。閱讀的時候需要不停的滾動捲軸。偵錯工具的時候如果想要找寫在底部的處理也需要進行檢索。

    DefaultSoftKeyboardJAJP.onKey當中採用了非常長的分支結構。

8.單例模式應用錯誤

    嚴重程度：高

    單例模式是用來確保全域訪問的時候都是只有一個唯一對象，從而保證訪問時的準確性。

    OpenWnn.mSelf為單利模式實現，但是它採用了private定義，表示只有它自己調用。那麼這就完全沒有必要了。使用this就可以了。

9.多餘的參數傳遞

    嚴重程度：中

    多餘的參數傳遞並不是引起Bug的原因，但是多餘的參數傳遞的確給調用帶來問題。

    OpenWnnJAJP.constructor(Context)。這個context參數傳遞的實在沒有必要。

10.傳回值採用全域變數

    嚴重程度：中

    方法傳回值不應該使用全域變數。如果方法會影響全域變數的值，那麼直接在外部存取全域變數即可。無需對方法使用傳回值。

    OpenWnnJAJP.commitText();

   以下是程式碼片段，其中的mStatus就是全域變數。

 1 private int commitText(boolean learn) {   2          if (isEnglishPrediction()) {   3              mComposingText.setCursor(ComposingText.LAYER1,   4 mComposingText.size(ComposingText.LAYER1));   5          }   6          int layer = mTargetLayer;   7          int cursor = mComposingText.getCursor(layer);   8          if (cursor == 0) {   9              return mStatus;  10          } 

    這種寫法表示代碼中的處理出現了問題，而mStatus是表示狀態的，commitText的傳回值也是表示狀態的。所以這個方法可以變成void類型的，然後在調用處直接使用mStatus。

11. 不必要的Guard代碼

    嚴重程度：中

    Guard代碼是防止發生Exception的代碼，比如
      if (instance != null)，防止NullPointerException；
      if (array.length >= accessIndex)防止ArrayIndexOutOfBoundsException；
      if (str != null && str.length() > end)防止發生StringIndexOutOfBoundsException；

      等等..

      但是如果代碼結構設計得好，instance可以確保不為空白，數組可以確保不越界

     
DefaultSoftKeyboard的下列代碼中的mCurrentKeyboard可以通過處理確保不為空白。對於這個代碼來說，mCurrentKeyboard可能為空白的情況是，開機啟動時，遇到了一個不可輸入的介面。而如果定義一個InvisibleKeyboard類的話，那麼在這種情況下把mCurrentKeyboard賦值為new InvisibleKeyboard()就可以了。

1     public View getCurrentView() {  2          if (mCurrentKeyboard == null) {  3              return null;  4          }  5          return mMainView;  6      }

 

12.冗餘的常量定義

    嚴重程度：中

    由於結構定義不佳，導致長篇累牘的常量列表，閱讀的時候總是想要跳過這部分。

    在OpenWnn中，對於各種鍵盤的按鍵採用了整數型的code來定義，而這些code到了代碼中就變成了一堆的常量。

    實際上這個可以通過結構上的改良來避免code定義的問題。

     比如：QWERTY鍵盤的xml檔案定義為：<row keys="q|w|e|r|t|y|u|i|o|p">，從而不必關心每個按鍵的代碼。

值得注意的是，連Google拼音IME都不採用Android定義的鍵盤xml結構，可見，OpenWnn採用了Android的鍵盤xml結構實在不是什麼高明的主意。

13.冗餘的臨時變數

    嚴重程度：中

    臨時變數是用來輔助演算法實現的，但是有些時候臨時變數的引入會使得代碼的複雜度變高。

    比如：DefaultSoftKeyboardJAJP.nextKeyMode中的found。

    單就這段處理來說，找到當前模式的下一個模式的處理其實還是結構上的問題不是具體演算法中是否出現found的問題。

14.無端的default

    嚴重程度：高

     喜歡用switch-case結構的人都知道，其中需要有default處理。但是對於一個取值範圍固定的代碼來說，default做什麼呢？很多人就是簡單地寫上default:break;放在那裡。但是，這種default就可能成為潛在的bug來源。比如，鍵盤的語言種類定義為7。如果傳進8的話會怎麼樣？

    而如果採用類定義的話，採用工廠來產生對應的類執行個體，則不會發生越界的情況。

15.意思不明的判定

    嚴重程度：高

    代碼中的判定作為分支處理的條件，但是對於if(parent.mComposingText.size(1)==0){這種代碼的==0是什麼意思呢？如果將該判定提取出來，改為if (!hasComposing()) {}就易懂得多了。而hasComposing的定義就是

1 private boolean hasComposing() {2     return parent.mComposingText.size(1) > 0;3 }

16. 演算法偏複雜

     嚴重程度：高

    複雜的演算法徒然增加程式碼數，使得閱讀困難。比如：下面的處理。

 1 private int getTableIndex(int keyCode) {   2          int index =   3              (keyCode == KEYCODE_JP12_1)     ?  0 :   4              (keyCode == KEYCODE_JP12_2)     ?  1 :   5              (keyCode == KEYCODE_JP12_3)     ?  2 :   6              (keyCode == KEYCODE_JP12_4)     ?  3 :   7              (keyCode == KEYCODE_JP12_5)     ?  4 :   8              (keyCode == KEYCODE_JP12_6)     ?  5 :   9              (keyCode == KEYCODE_JP12_7)     ?  6 :  10              (keyCode == KEYCODE_JP12_8)     ?  7 :  11              (keyCode == KEYCODE_JP12_9)     ?  8 :  12              (keyCode == KEYCODE_JP12_0)     ?  9 :  13              (keyCode == KEYCODE_JP12_SHARP) ? 10 :  14              (keyCode == KEYCODE_JP12_ASTER) ? 11 :  15              0;  16          return index;  17  } 18  

這段代碼可以簡化成為：

1 private int getTableIndex(int keyCode) {2     return keyCode - KEYCODE_JP12_1;3 }

類似的，OpenWnn.searchToggleCharacter也是一個邏輯上偏於複雜的代碼。

原來的代碼：

 1     protected String searchToggleCharacter(String prevChar, String[] toggleTable, boolean reverse) {   2          for (int i = 0; i < toggleTable.length; i++) {   3              if (prevChar.equals(toggleTable[i])) {   4                  if (reverse) {   5                      i--;   6                      if (i < 0) {   7                          return toggleTable[toggleTable.length - 1];   8                      } else {   9                          return toggleTable[i];  10                      }  11                  } else {  12                      i++;  13                      if (i == toggleTable.length) {  14                          return toggleTable[0];  15                      } else {  16                          return toggleTable[i];  17                      }  18                  }  19              }  20          }  21          return null;  22      } 

 

可以簡單地寫成如下形式

1 protected String searchToggleCharacter(String prevChar, String[] toggleTable, boolean reverse) {  2     for (int i = 0; i < toggleTable.length; i++) { 3         if (prevChar.equals(toggleTable[i])) {4             return toggleTable[(i + (reverse ? 1 : -1) + toggleTable.length) % toggleTable.length];5         }6     }
7     return null;8 }

 

17.同一個意思有若干個變數來表示

    嚴重程度：非常高

    同一個意思如果有多個變數來表示，那麼可能產生不一致。由於不一致而產生的問題都會是比較嚴重的Bug。

    在OpenWnn中存在很多這種情況。

     A.英語模式  除了KeyMode表示之外，還有一個isEnglishPrediction——引擎模式來表示。
     B.候選全屏  除了TextCandidatesViewManager.mViewType之外，還有TextCandidatesViewManager.mIsFullView 。
     C.螢幕縱橫  除了DefaultSoftKeyboard.mDisplayMode之外，還有TextCandidatesViewManager.mPortrait。

 

18.無用的private方法

    嚴重程度：低

    private方法只能給類內部使用，但是如果private方法只是返回某個內部變數的話，那麼這個方法的定義就是畫蛇添足，多此一舉。

     TextCandidatesViewManager.getCandidateMinimumWidth和TextCandidatesViewManager.getCandidateMinimumHeight方法就是這種類型的。

 

19.用錯API

   嚴重程度：中

   某些API有一定的限制，但是使用的時候如果並不知道，那麼就會在系統中遺留Bug。

   OpenWnn中的TextCandidatesViewManager.mFormat用來格式化輸出帶逗號的數字候補，但是當數位大小超過一定限制的時候就會發生四捨五入的情況。這恐怕是原作者並不知道的情況。另外，帶有小數點的時候也恐怕不會得到正確的數字吧。

 20.輸出型參數

   嚴重程度：中

   作為Java語言的一個特性，允許參數作為輸出型參數使用，即方法內部可以變更參數的值，並且產生效果。

   但是輸出型參數如果不做以明確的標記，則容易出現調用錯誤，而導致資料誤變更。

   KanaConverter.createCandidateString就是這樣一個方法，它的第三個參數StringBuffer就是一個輸出型參數。但是它的方法聲明以及注釋中並沒有說明這一點。這個方法的簽名式如果改成下面這樣就會更好懂了。

 

private StringBuffer createCandidateString(String input, 
        HashMap<String,String> map) throws InconvertableException

其中的InconvertableException是自訂的異常，用以替代原來的boolean型傳回值。