審查Java代碼的十一種常見錯誤

代碼審查是消滅Bug最重要的方法之一，這些審查在大多數時候都特別奏效。由於代碼審查本身所針對的對象，就是俯瞰整個代碼在測試過程中的問題和Bug。並且，代碼審查對消除一些特別細節的錯誤大有裨益，尤其是那些能夠容易在閱讀代碼的時候發現的錯誤，這些錯誤往往不容易通過機器上的測試識別出來。本文就常見的Java代碼中容易出現的問題提出一些建設性建議，以便您在審查代碼的過程中注意到這些常見的細節性錯誤。 

　　通常給別人的工作挑錯要比找自己的錯容易些。別樣視角的存在也解釋了為什麼作者需要編輯，而運動員需要教練的原因。不僅不應當拒絕別人的批評，我們應該歡迎別人來發現並指出我們的編程工作中的不足之處，我們會受益匪淺的。 　正規的代碼審查(code inspection)是提高代碼質量的最強大的技術之一，代碼審查—由同事們尋找代碼中的錯誤—所發現的錯誤與在測試中所發現的錯誤不同，因此兩者的關系是互補的，而非競爭的。 

　　如果審查者能夠有意識地尋找特定的錯誤，而不是靠漫無目的的浏覽代碼來發現錯誤，那麼代碼審查的效果會事半功倍。在這篇文章中，我列出了11個Java編程中常見的錯誤。你可以把這些錯誤添加到你的代碼審查的檢查列表(checklist)中，這樣在經過代碼審查後，你可以確信你的代碼中不再存在這類錯誤了。

　　一、常見錯誤1# ：多次拷貝字符串

　　測試所不能發現的一個錯誤是生成不可變(immutable)對象的多份拷貝。不可變對象是不可改變的，因此不需要拷貝它。最常用的不可變對象是String。

　　如果你必須改變一個String對象的內容，你應該使用StringBuffer。下面的代碼會正常工作： 

String s = new String ("Text here");
　　但是，這段代碼性能差，而且沒有必要這麼復雜。你還可以用以下的方式來重寫上面的代碼： 

String temp = "Text here"; 
String s = new String (temp);
　　但是這段代碼包含額外的String，並非完全必要。更好的代碼為： 

String s = "Text here";
　　二、常見錯誤2#： 沒有克隆(clone)返回的對象

　　封裝(encapsulation)是面向對象編程的重要概念。不幸的是，Java為不小心打破封裝提供了方便——Java允許返回私有數據的引用(reference)。下面的代碼揭示了這一點： 

import Java.awt.Dimension; 
/***Example class.The x and y values should never*be negative.*/ 
public class Example{ 
　 private Dimension d = new Dimension (0, 0); 
　 public Example (){ } 

　 /*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ 
　 public synchronized void setValues (int height,int width) throws IllegalArgumentException{ 
　　 if (height < 0 || width < 0) 
　　　 throw new IllegalArgumentException(); 
　　　 d.height = height; 
　　 d.width = width; 
　 }

　 public synchronized Dimension getValues(){ 
　　 // Ooops! Breaks encapsulation 
　　 return d; 
　 } 
}
　　Example類保證了它所存儲的height和width值永遠非負數，試圖使用setValues()方法來設置負值會觸發異常。不幸的是，由於getValues()返回d的引用，而不是d的拷貝，你可以編寫如下的破壞性代碼： 

Example ex = new Example(); 
Dimension d = ex.getValues(); 
d.height = -5; 
d.width = -10;
　　現在，Example對象擁有負值了！如果getValues() 的調用者永遠也不設置返回的Dimension對象的width 和height值，那麼僅憑測試是不可能檢測到這類的錯誤。

　　不幸的是，隨著時間的推移，客戶代碼可能會改變返回的Dimension對象的值，這個時候，追尋錯誤的根源是件枯燥且費時的事情，尤其是在多線程環境中。

　　更好的方式是讓getValues()返回拷貝： 

public synchronized Dimension getValues(){ 
return new Dimension (d.x, d.y); 
}
　　現在，Example對象的內部狀態就安全了。調用者可以根據需要改變它所得到的拷貝的狀態，但是要修改Example對象的內部狀態，必須通過setValues()才可以。

　　三、常見錯誤3#：不必要的克隆

　　我們現在知道了get方法應該返回內部數據對象的拷貝，而不是引用。但是，事情沒有絕對： 

/*** Example class.The value should never * be negative.*/
public class Example{ 
　 private Integer i = new Integer (0); 
　 public Example (){ } 

　 /*** Set x. x must be nonnegative* or an exception will be thrown*/ 
　 public synchronized void setValues (int x) throws IllegalArgumentException{ 
　　 if (x < 0) 
　　　 throw new IllegalArgumentException(); 
　　　 i = new Integer (x); 
　 }

　 public synchronized Integer getValue(){ 
　　 // We can’t clone Integers so we makea copy this way. 
　　 return new Integer (i.intValue()); 
　 } 
}
　　這段代碼是安全的，但是就象在錯誤1#那樣，又作了多余的工作。Integer對象，就象String對象那樣，一旦被創建就是不可變的。因此，返回內部Integer對象，而不是它的拷貝，也是安全的。

　　方法getValue()應該被寫為： 

public synchronized Integer getValue(){ 
// ’i’ is immutable, so it is safe to return it instead of a copy. 
return i; 
}
　　Java程序比C++程序包含更多的不可變對象。JDK 所提供的若干不可變類包括： 

　　·Boolean 
　　 ·Byte 
　　 ·Character 
　　 ·Class 
　　 ·Double 
　　 ·Float 
　　 ·Integer 
　　 ·Long 
　　 ·Short 
　　 ·String 
　　 ·大部分的Exception的子類

　　四、常見錯誤4# ：自編代碼來拷貝數組

　　Java允許你克隆數組，但是開發者通常會錯誤地編寫如下的代碼，問題在於如下的循環用三行做的事情，如果采用Object的clone方法用一行就可以完成： 

public class Example{ 
　 private int[] copy; 
　 /*** Save a copy of ’data’. ’data’ cannot be null.*/ 
　 public void saveCopy (int[] data){ 
　　 copy = new int[data.length]; 
　　 for (int i = 0; i < copy.length; ++i) 
　　　 copy[i] = data[i]; 
　 } 
}
　　這段代碼是正確的，但卻不必要地復雜。saveCopy()的一個更好的實現是： 

void saveCopy (int[] data){ 
　 try{ 
　　 copy = (int[])data.clone(); 
　 }catch (CloneNotSupportedException e){ 
　　 // Can’t get here. 
　 } 
}

　　如果你經常克隆數組，編寫如下的一個工具方法會是個好主意：

static int[] cloneArray (int[] data){ 
　 try{ 
　　 return(int[])data.clone(); 
　 }catch(CloneNotSupportedException e){ 
　　 // Can’t get here. 
　 } 
}
　　這樣的話，我們的saveCopy看起來就更簡潔了： 

void saveCopy (int[] data){ 
　 copy = cloneArray ( data); 
}

 

五、常見錯誤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需要一些技巧。

　　小結

　　我在代碼審查的時候至少遇到過一次這些錯誤，我自己也犯過其中的幾個錯誤。好消息是只要你知道你在找什麼錯誤，那麼代碼審查就很容易管理，錯誤也很容易被發現和修改。即便你找不到時間來進行正規的代碼審查，以自審的方式把這些錯誤從你的代碼中根除會大大節省你的調試時間。花時間在代碼審查上是值得的。