代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 '~
H`Ffd.
D Q30\b"gU
Q6D>(H#"0
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 eL~3CAV{
)[oP`Z
%}e['d h
r8?p6E
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 1wFW&|>1
#:By/9}-
xy
b=7
mP Hto-=fB
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 qoOwR[NDcq
qYJ<I'Ux O
+Gg|BTTL/
~_Fx2T:X
一、常见错误1# :多次拷贝字符串 _VVq&t}
_",<at
l i)6^f#
Il Qk W<
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 ;S
\s&. u
W@ &a
0KTO)K
@_?2iN?4Z
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: ar#73f
)z\#
c BZ,"kp-
Xdx8HB@L
String s = new String ("Text here"); \Oq8kJ=
*hru);OJr
,
^K.J29
c?e-2Dp(
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: YoW)]n
S3l^h4
wU>Fz*
/,\U*'-
String temp = "Text here"; 1Y*k"[?dW
String s = new String (temp); 8lzoiA_9
!+A%`m
[^s;Ggi9
dW%t ph
但是这段代码包含额外的String,并非完全必要。更好的代码为: G;flj}z
q&J5(9]O|L
$y&W:
D =mmBo
String s = "Text here"; pZ}B/j
n1{[CCee@
,dR.Sacv
?&;_>0P
二、常见错误2#: 没有克隆(clone)返回的对象 c8YbBdk'
LXh}U>a9
sYBmL]Hr
n@xQ-v
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: nq HpYb6I0
`YC7+`q
!u@P\8M}
`BK b60
import java.awt.Dimension; "gJ.mhHX
/***Example class.The x and y values should never*be negative.*/ NIVR;gm
public class Example{ Ht4O5yl"
private Dimension d = new Dimension (0, 0); 2H}y1bkW
public Example (){ } Vj 9X6u}{
z4Zm%
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ S4`X^a}pY
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ `
PQQU~^
if (height < 0 || width < 0) tiE|%jOzt
throw new IllegalArgumentException(); 5{k,/Z[L
d.height = height; NI#]#yM+
d.width = width; Fz';H
} "A"YgD#t
Qy0w'L/@
public synchronized Dimension getValues(){ bf0,3~G,P
// Ooops! Breaks encapsulation F5RL+rU(h
return d; T>'O[=UWh
} d}zh.O5P!
} ^n0;Q$\
<O
0Q]`i
XQ9W
y
V%s7*`U
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: )f|`mM4DW!
+1YEOOfVY
OyVP_Yx,V
Lo1ySLo$G
Example ex = new Example(); MGsQF #6]
Dimension d = ex.getValues(); 05R"/r*
d.height = -5; myR{}G
d.width = -10; Lm~<BBp.
;7qIm83
pQBn8H|Y
#| _VN %!
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 m..ajYSQ
&{.IUg
XJ\R'?j
DOJydYds
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 9>w~B|/
dhob]8b
IZj`*M%3
olv?$]
更好的方式是让getValues()返回拷贝: o& FOp'
rL1yq|]I
HvG %##
'~&W'='b;
public synchronized Dimension getValues(){ @6yc^DAA
return new Dimension (d.x, d.y); ;6P>S4`w
} ?iP7Ki
Pgr2S I
(T#$0RFq
7/IL"
D
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 Q}@t'
xyL)'C
{WrEe7dLy
0fXMY-$I
三、常见错误3#:不必要的克隆 K 77iv
G-T^1?
* )<+u~
>|A,rE^Ojt
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: S[3"?$3S
,~naKd.ZY
e9{0hw7
dgpE3
37Lt
/*** Example class.The value should never * be negative.*/ "jum*<QZz
public class Example{ PiKP.
private Integer i = new Integer (0); o@zxzZWg
public Example (){ } :TU|:2+
aNEah
/*** Set x. x must be nonnegative* or an exception will be thrown*/ z qq
public synchronized void setValues (int x) throws IllegalArgumentException{ VQHB}Y@^
if (x < 0) \uOM,98xS
throw new IllegalArgumentException(); '_G\_h}5
i = new Integer (x); q k^FyZ<
} I;t@wbY,
|ZH(Z}m
public synchronized Integer getValue(){ '-%1ILK$3r
// We can’t clone Integers so we makea copy this way. .@,t}:lD
return new Integer (i.intValue()); UmWXv#q\l
} kP%hgZ
} UA8hYWRP
losqc *|
[
@eA o>
P 0.cF]<m
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 eZPeyYX
)*]A$\Oc[
R7Y_ 7@p
x8rg/y
方法getValue()应该被写为: =:s`C,l.4
US ALoe
;nBf
Wn=sF,c
public synchronized Integer getValue(){ c9-$^yno
// ’i’ is immutable, so it is safe to return it instead of a copy. L5FOlzn
return i; [_'A(.
} y{hg4|\
}:IIk-JoC
GP%V(HhN
}N[X<9^Z
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: zkRAul32|
U9:)qvMXe
t`H1]`c?
D!o[Sm}JO[
?Boolean ~9+01UU^
?Byte d^}p#7mB\
?Character H]/~
#a
?Class " !EnQB=
?Double M_ukG~/
?Float o0R?vnA=
?Integer !vgY3S0?rq
?Long ;0 B1P|7zK
?Short [LnPV2@e
?String Vn^GJ'^
?大部分的Exception的子类 0P5VbDv$r7
A7X
a
$yASWz
{}YA7M:L
四、常见错误4# :自编代码来拷贝数组 Da(k>vR@4
TRm#H$
'Bue*
h:8P9WhWF
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: +06{5-,
:VT%d{Vp_
9!_,A d;3
g{]6*`/Z
public class Example{ #%;Uh
private int[] copy; .]vb\NBK7
/*** Save a copy of ’data’. ’data’ cannot be null.*/ |eu8;~A
public void saveCopy (int[] data){ ytIPY7E
copy = new int[data.length]; oVpZR$
for (int i = 0; i < copy.length; ++i) WoZU} T-
copy = data; ;W?#l$R
} RK!9(^Ja
} Mr)t>4
h =A
"bhK%N;
TGF$zvd
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: [K3
te
|c/=9Bb
z{W Cw
u4Nh_x8\Nr
void saveCopy (int[] data){ :|W=2(>
try{ U T\4Xk<
copy = (int[])data.clone(); /yG7!k]Eg
}catch (CloneNotSupportedException e){ 12Oa_6<\0;
// Can’t get here. inGUN??
} .}\8Y=
} *K|~]r(F?
u}nS dZC
>_2~uF@pb
(fF8)4l
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: wo0j/4o
O^MI073Q>t
6MVu"0#
vS8&,wJ!
static int[] cloneArray (int[] data){ 7% D 4
try{ r E m/Q!
return(int[])data.clone(); v])ew|
}catch(CloneNotSupportedException e){ OE@[a
// Can’t get here. Q7aPW\-
} Jo {:]:
} \|0z:R;X
?/o 8f7Z
4Im>2)
R&Lqaek&W
这样的话,我们的saveCopy看起来就更简洁了:
mWv$eR
KkCGL*]K
sE[`x^1'8
n2K1X!E$
void saveCopy (int[] data){ d=vuy
copy = cloneArray ( data); G<7M;vRvP
} 2f[;U"
r84^/+"T
~lo43$)^
C+TB>~Gv`
五、常见错误5#:拷贝错误的数据 wtYgHC}X
Cy[G7A%
p*b_"aF 1
>%tG[jb
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: |SOLC
}MQ:n8
relt7 sK
q!c=f!U?\l
import java.awt.Dimension; zGtJ@HbB
/*** Example class. The height and width values should never * be @s1T|}AJ
negative. */ 6M
>@DRZ'|
public class Example{ 4Fft[S(
static final public int TOTAL_VALUES = 10; ]Ucw&B*@
private Dimension[] d = new Dimension[TOTAL_VALUES]; 8* A%k1+
public Example (){ } v@=qVwX
@-sWXz*W
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ ,>-j Ztm
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ P PJ^;s
if (height < 0 || width < 0) p^8a<e?f~f
throw new IllegalArgumentException(); xxur4@p!
if (d[index] == null) 8oJl ]
d[index] = new Dimension(); y >=Y
d[index].height = height; uN)c!='I
d[index].width = width; {32m&a
} S~3|1Hw*tN
public synchronized Dimension[] getValues() Rge>20uTl$
throws CloneNotSupportedException{ wOf8\s1
return (Dimension[])d.clone(); UH MJ(.Wa-
} +Vk L?J
} 8._uwA<[
N0p6xg~
a^%)6E.[,
p3A9<g
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为:
LFax$CZc
T]%-Ri
Y!L-5|G
\E?3nQM
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ nB`|VYmOP1
Dimension[] copy = (Dimension[])d.clone(); %&6QUv^
for (int i = 0; i < copy.length; ++i){ 6.)ug7aF
// NOTE: Dimension isn’t cloneable. LTe ({6l0
if (d != null) 8{ZTHY-
copy = new Dimension (d.height, d.width); @/s|<*
} 5?^#v
return copy; r]!#v{#.
} D"pT?\kO
z6R|1L 1
p-iFe\+
94w)Yln
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: h^ Cm\V
{IgH0+z
$eFMn$o
;M.Q=#;E
public void store (int[] data) throws CloneNotSupportedException{ ?>B?*IK!
this.data = (int[])data.clone(); t"4* ]S
// OK p3Ux%/ZqPV
} O.+J%],
ZPH_s^
7Y8~")f
<YW)8J
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: Z{B
e
W4o8]&A
r.eK;
\x-2qlZ
public void wrongStore (int[][] data) throws CloneNotSupportedException{ RH FRN&RU$
this.data = (int[][])data.clone(); // Not OK! |<u+Xi
~
} cA Nt7
public void rightStore (int[][] data){ cTq@"v di
// OK! 4G,FJjE`p
this.data = (int[][])data.clone(); gHPJiiCv
for (int i = 0; i < data.length; ++i){ @mCe{r*`
if (data != null) MSmr7%g3D
this.data = (int[])data.clone(); $bG*f*w
} Br!;Ac&N
} HS<Jp44
><}nZ7
7Vy_Cec1
u1 Q;M`+>
_[R(9KyF0f
六、常见错误6#:检查new 操作的结果是否为null M*0^<e~]F
q? ">
~pZ0B#K
J
&{? M} 2I
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: 486\a
b WNa6x
V| V9.
rC!O}(4t%$
Integer i = new Integer (400); VFf;|PHS
if (i == null) Q2 !GWz$
throw new NullPointerException(); f5*qlQJFz\
ZR\N~.
C7dq=(p&
Q#3}AO
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 @4y?XL(n
,cNe-KJk
NVx>^5QV
{N}az"T4f
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 7n#-3#_mG
b#?sx"z
七、常见错误7#:用== 替代.equals ``CM7|)>`
7"'RE95
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: ~-k,$J?7
#//xOL3J
]R""L<K%HF
C~T,[U
int x = 4; 4*}&nmW
int y = 5; S'!&,Dxq^
if (x == y) \(pwHNSafk
System.out.println ("Hi"); v)Y)tu>
// This ’if’ test won’t compile. K@7%i|H
if (x.equals (y)) U*~-\jN1pb
System.out.println ("Hi"); B2kZ_4rB
fx|d"VF[
t}k:wzZ@
b@CjnAZ
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 )G)6D"5,+G
RyK~"CWT
|p/*OFC6
/p<9C?
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。
`o#(YEu
4,;*sc 6*
LVg#E*J
/[_aK0U3
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 )IcSdS0@M
5! );4+
=;-C;gn:w
=Smd/'`_
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 _$R=F/88
>h8m)Q
,^G+<T6
ZRf9 'UwS
八、常见错误8#: 混淆原子操作和非原子操作 u~OlJ1V
T!,5dt8L
Bg),Q8\I
<\epj=OclV
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: fJr
EDj4(
h|]cZMGo
OpaRQ=
:j`f%Vg~x
public class Example{ KfjWZ4{v
private int value; // More code here... _+48(QF<