五、常見錯誤5#:拷貝錯誤的數據
有時候程序員知道必須返回一個拷貝,但是卻不小心拷貝了錯誤的數據。由於僅僅做了部分的數據拷貝工作,下面的代碼與程序員的意圖有偏差:
- import Java.awt.Dimension;
- /*** Example class. The height and width values should never * be
- negative. */
- public class Example{
- static final public int TOTAL_VALUES = 10;
- private Dimension[] d = new Dimension[TOTAL_VALUES];
- public Example (){ }
- /*** Set height and width. Both height and width must be nonnegative * or
- an exception will be thrown. */
- public synchronized void setValues (int index, int height, int width)
- throws IllegalArgumentException{
- if (height < 0 || width < 0)
- throw new IllegalArgumentException();
- if (d[index] == null)
- d[index] = new Dimension();
- d[index].height = height;
- d[index].width = width;
- }
- public synchronized Dimension[] getValues()
- throws CloneNotSupportedException{
- return (Dimension[])d.clone();
- }
- }
這兒的問題在於getValues()方法僅僅克隆了數組,而沒有克隆數組中包含的Dimension對象,因此,雖然調用者無法改變內部的數組使其元素指向不同的Dimension對象,但是調用者卻可以改變內部的數組元素(也就是Dimension對象)的內容。方法getValues()的更好版本為:
- public synchronized Dimension[] getValues() throws CloneNotSupportedException{
- Dimension[] copy = (Dimension[])d.clone();
- for (int i = 0; i < copy.length; ++i){
- // NOTE: Dimension isn’t cloneable.
- if (d != null)
- copy[i] = new Dimension (d[i].height, d[i].width);
- }
- return copy;
- }
在克隆原子類型數據的多維數組的時候,也會犯類似的錯誤。原子類型包括int,float等。簡單的克隆int型的一維數組是正確的,如下所示:
- public void store (int[] data) throws CloneNotSupportedException{
- this.data = (int[])data.clone();
- // OK
- }
拷貝int型的二維數組更復雜些。Java沒有int型的二維數組,因此一個int型的二維數組實際上是一個這樣的一維數組:它的類型為int[]。簡單的克隆int[][]型的數組會犯與上面例子中getValues()方法第一版本同樣的錯誤,因此應該避免這麼做。下面的例子演示了在克隆int型二維數組時錯誤的和正確的做法:
- public void wrongStore (int[][] data) throws CloneNotSupportedException{
- this.data = (int[][])data.clone(); // Not OK!
- }
- public void rightStore (int[][] data){
- // OK!
- this.data = (int[][])data.clone();
- for (int i = 0; i < data.length; ++i){
- if (data != null)
- this.data[i] = (int[])data[i].clone();
- }
- }
六、常見錯誤6#:檢查new 操作的結果是否為null
Java編程新手有時候會檢查new操作的結果是否為null。可能的檢查代碼為:
- Integer i = new Integer (400);
- if (i == null)
- throw new NullPointerException();
檢查當然沒什麼錯誤,但卻不必要,if和throw這兩行代碼完全是浪費,他們的唯一功用是讓整個程序更臃腫,運行更慢。
C/C++程序員在開始寫java程序的時候常常會這麼做,這是由於檢查C中malloc()的返回結果是必要的,不這樣做就可能產生錯誤。檢查C++中new操作的結果可能是一個好的編程行為,這依賴於異常是否被使能(許多編譯器允許異常被禁止,在這種情況下new操作失敗就會返回null)。在Java 中,new 操作不允許返回null,如果真的返回null,很可能是虛擬機崩潰了,這時候即便檢查返回結果也無濟於事。
七、常見錯誤7#:用== 替代.equals
在Java中,有兩種方式檢查兩個數據是否相等:通過使用==操作符,或者使用所有對象都實現的.equals方法。原子類型(int, flosat, char 等)不是對象,因此他們只能使用==操作符,如下所示:
- int x = 4;
- int y = 5;
- if (x == y)
- System.out.println ("Hi");
- // This ’if’ test won’t compile.
- if (x.equals (y))
- System.out.println ("Hi");
對象更復雜些,==操作符檢查兩個引用是否指向同一個對象,而equals方法則實現更專門的相等性檢查。
更顯得混亂的是由Java.lang.Object 所提供的缺省的equals方法的實現使用==來簡單的判斷被比較的兩個對象是否為同一個。
許多類覆蓋了缺省的equals方法以便更有用些,比如String類,它的equals方法檢查兩個String對象是否包含同樣的字符串,而Integer的equals方法檢查所包含的int值是否相等。
大部分時候,在檢查兩個對象是否相等的時候你應該使用equals方法,而對於原子類型的數據,你用該使用==操作符。
八、常見錯誤8#: 混淆原子操作和非原子操作
Java保證讀和寫32位數或者更小的值是原子操作,也就是說可以在一步完成,因而不可能被打斷,因此這樣的讀和寫不需要同步。以下的代碼是線程安全(thread safe)的:
- public class Example{
- private int value; // More code here...
- public void set (int x){
- // NOTE: No synchronized keyWord
- this.value = x;
- }
- }
不過,這個保證僅限於讀和寫,下面的代碼不是線程安全的:
- public void increment (){
- // This is effectively two or three instructions:
- // 1) Read current setting of ’value’.
- // 2) Increment that setting.
- // 3) Write the new setting back.
- ++this.value;
- }
在測試的時候,你可能不會捕獲到這個錯誤。首先,測試與線程有關的錯誤是很難的,而且很耗時間。其次,在有些機器上,這些代碼可能會被翻譯成一條指令,因此工作正常,只有當在其它的虛擬機上測試的時候這個錯誤才可能顯現。因此最好在開始的時候就正確地同步代碼:
- public synchronized void increment (){
- ++this.value;
- }
九、常見錯誤9#:在catch 塊中作清除工作
一段在catch塊中作清除工作的代碼如下所示:
- OutputStream os = null;
- try{
- os = new OutputStream ();
- // Do something with os here.
- os.close();
- }catch (Exception e){
- if (os != null)
- os.close();
- }
盡管這段代碼在幾個方面都是有問題的,但是在測試中很容易漏掉這個錯誤。下面列出了這段代碼所存在的三個問題:
1.語句os.close()在兩處出現,多此一舉,而且會帶來維護方面的麻煩。
2.上面的代碼僅僅處理了Exception,而沒有涉及到Error。但是當try塊運行出現了Error,流也應該被關閉。
3.close()可能會拋出異常。
上面代碼的一個更優版本為:
- OutputStream os = null;
- try{
- os = new OutputStream ();
- // Do something with os here.
- }finally{
- if (os != null)
- os.close();
- }
這個版本消除了上面所提到的兩個問題:代碼不再重復,Error也可以被正確處理了。但是沒有好的方法來處理第三個問題,也許最好的方法是把close()語句單獨放在一個try/catch塊中。
十、常見錯誤10#: 增加不必要的catch 塊
一些開發者聽到try/catch塊這個名字後,就會想當然的以為所有的try塊必須要有與之匹配的catch塊。
C++程序員尤其是會這樣想,因為在C++中不存在finally塊的概念,而且try塊存在的唯一理由只不過是為了與catch塊相配對。
增加不必要的catch塊的代碼就象下面的樣子,捕獲到的異常又立即被拋出:
- try{
- // Nifty code here
- }catch(Exception e){
- throw e;
- }finally{
- // Cleanup code here
- }
不必要的catch塊被刪除後,上面的代碼就縮短為:
- try{
- // Nifty code here
- }finally{
- // Cleanup code here
- }
常見錯誤11#;沒有正確實現equals,hashCode,或者clone 等方法
方法equals,hashCode,和clone 由Java.lang.Object提供的缺省實現是正確的。不幸地是,這些缺省實現在大部分時候毫無用處,因此許多類覆蓋其中的若干個方法以提供更有用的功能。但是,問題又來了,當繼承一個覆蓋了若干個這些方法的父類的時候,子類通常也需要覆蓋這些方法。在進行代碼審查時,應該確保如果父類實現了equals,hashCode,或者clone等方法,那麼子類也必須正確。正確的實現equals,hashCode,和clone需要一些技巧。
小結
我在代碼審查的時候至少遇到過一次這些錯誤,我自己也犯過其中的幾個錯誤。好消息是只要你知道你在找什麼錯誤,那麼代碼審查就很容易管理,錯誤也很容易被發現和修改。即便你找不到時間來進行正規的代碼審查,以自審的方式把這些錯誤從你的代碼中根除會大大節省你的調試時間。花時間在代碼審查上是值得的。