编程错题本——解析OpenWnn（3）结构问题

结构问题是所有问题中最严重的问题，这里的很多问题都反映了作者缺少基本常识。

代码的结构就像房屋的结构一样，是支撑整个代码的框架。如果房屋的结构不够结实，房屋就会倒塌。如果代码的结构不够良好那么修改Bug或者对应需求变更的时候就要付出昂贵的代价。

1. 缺少MVC结构

    严重程度：非常高

     输入法和其他应用程序一样，都是由：数据(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就是全局变量。

private int commitText(boolean learn) {  
         if (isEnglishPrediction()) {  
             mComposingText.setCursor(ComposingText.LAYER1,  
mComposingText.size(ComposingText.LAYER1));  
         }  
         int layer = mTargetLayer;  
         int cursor = mComposingText.getCursor(layer);  
         if (cursor == 0) {  
             return mStatus;  
         } 

    这种写法表示代码中的处理出现了问题，而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()就可以了。

    public View getCurrentView() {  
         if (mCurrentKeyboard == null) {  
             return null;  
         }  
         return mMainView;  
     }

 

12.冗余的常量定义

    严重程度：中

    由于结构定义不佳，导致长篇累牍的常量列表，阅读的时候总是想要跳过这部分。

    在OpenWnn中，对于各种键盘的按键采用了整数型的code来定义，而这些code到了代码中就变成了一堆的常量。

    实际上这个可以通过结构上的改良来避免code定义的问题。

     比如：QWERTY键盘的xml文件定义为：<row keys="q|w|e|r|t|y|u|i|o|p">，从而不必关心每个按键的代码。

值得注意的是，连谷歌拼音输入法都不采用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的定义就是

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

16. 算法偏复杂

     严重程度：高

    复杂的算法徒然增加代码行数，使得阅读困难。比如：下面的处理。

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

这段代码可以简化成为：

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

类似的，OpenWnn.searchToggleCharacter也是一个逻辑上偏于复杂的代码。

原来的代码：

    protected String searchToggleCharacter(String prevChar, String[] toggleTable, boolean reverse) {  
         for (int i = 0; i < toggleTable.length; i++) {  
             if (prevChar.equals(toggleTable[i])) {  
                 if (reverse) {  
                     i--;  
                     if (i < 0) {  
                         return toggleTable[toggleTable.length - 1];  
                     } else {  
                         return toggleTable[i];  
                     }  
                 } else {  
                     i++;  
                     if (i == toggleTable.length) {  
                         return toggleTable[0];  
                     } else {  
                         return toggleTable[i];  
                     }  
                 }  
             }  
         }  
         return null;  
     } 

 

可以简单地写成如下形式

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

 

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型返回值。