代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 Lkn4<'un
Tg0CE60"
$#e1SS32
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 9gy(IRGq/
L0L2Ns
y \D=Z
N@
<.bRf
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 .fp&MgiQ
5pfYEofK[
H>XFz(LWh
I-kWS4
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 5wv fF.v
!X]8dyW
uH:YKH':/
\F{:5,Du)
一、常见错误1# :多次拷贝字符串 :5b0np!
~E)fpGJ
WF[bO7:
F'FP0t!S
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 4t*so~
2: SO_O4C
v7,$7@$:\
6~xBi(m`
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: Ls}7VKl'
l$XPIC~H
Rko M~`CT
XKS8K4"
String s = new String ("Text here"); 2'] KTHm
/TV=$gB`
Dvc&RG
D d,2;#_
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: 5)UQWnd5
dg_G s>?2
> 'i
A6!F@Ic[
String temp = "Text here"; A&"%os
String s = new String (temp); H
C0w;MG)
?6"{!s{v
.4-,_`T?
n}?wVfEy
但是这段代码包含额外的String,并非完全必要。更好的代码为: \)/yC74r7(
!5Sd2<N
+?dl`!rE
VUwC-)
String s = "Text here"; xfFg,9w8
gE])!GMM3
%IY``r)j
{A:j[
二、常见错误2#: 没有克隆(clone)返回的对象 [{
~TcT
t9cl"F=
;
)Eo7?]-
F_H82BE+3
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: S1S;F9F
A/}W&bnluD
bt$)Xu<R
y*23$fj(
import java.awt.Dimension; ?LK 2g
/***Example class.The x and y values should never*be negative.*/ [yS#O\$'e
public class Example{ \ck+GW4&
private Dimension d = new Dimension (0, 0); i'#Gy,R
public Example (){ } B9,^mE#
)]htm&q5
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ j)C:$
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ XYrJ/!*.
if (height < 0 || width < 0) SF*n1V3hx
throw new IllegalArgumentException(); 3W_PE+:Kr
d.height = height; D5,P)[
d.width = width; j+-P :xvP
} >znRyQ~bM
-E4XIn
public synchronized Dimension getValues(){ Sa1l=^
// Ooops! Breaks encapsulation 7 msAhz
return d; $F'>yop2b
} vVl; |
} m P'^%TE
kwpK1R4zs
BV#78,8(
hC <O`|lF
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: v<Kmq-b
:'iYxhM.V
=#gEB#$x:
H1n1-!%d
Example ex = new Example(); NMOut@
Dimension d = ex.getValues(); JM- t<.
d.height = -5; \>QF(J [8
d.width = -10; c%m3}mrb
/3 B
$(
re?s.djT
}a
AH
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 ig}A9j?]
NKb1LbnZ*y
)Gw~XtB2
Q6'x\
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 rgmF: C
c(;a=n(E#
3jB$2: #
YuZ"s55zU{
更好的方式是让getValues()返回拷贝: O3DmNq$dz
7\FXz'hA
,JU@|`
OyV<u@[i
public synchronized Dimension getValues(){ L@`ouQ"sa
return new Dimension (d.x, d.y); ~w8JH2O
} D^%^xq)E
'R`tLN
Suk
Sf5X3,Uw
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 &F STpBu
;2'q_Btk4
D(-yjY8aG
4SPy28<f
三、常见错误3#:不必要的克隆 h.O$]:N
s*U1
$un?0S
&nBa=Enf
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: J]f3CU,<N
<\kr1qHH
iu&wO<)+?
AKMm&(fh%
/*** Example class.The value should never * be negative.*/ >SPh2[f
public class Example{ oF(Lji?m
private Integer i = new Integer (0); ;qH O OT
public Example (){ } yE[#ze
r'QnX;99T
/*** Set x. x must be nonnegative* or an exception will be thrown*/ ok|qyN+
public synchronized void setValues (int x) throws IllegalArgumentException{ V,rq0xW
if (x < 0) 3gd&i
throw new IllegalArgumentException(); OO[F E3F
i = new Integer (x); -'~LjA(
} b#7{{@H
S26MDLk`R3
public synchronized Integer getValue(){ ~/.7l8)
// We can’t clone Integers so we makea copy this way. Vz6Qxd{m3
return new Integer (i.intValue()); aaD;jxT&M|
} Reatdh
} S[WG$
&gzCteS
e[hcJz!D
`{qG1
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 C z\Pp q
t%F0:SH
)iFJz/n>
sc,Xw:YO
方法getValue()应该被写为: (}}S9 K
W`c'=c
M Y|w
yj^+G
public synchronized Integer getValue(){ $56,$K`H
// ’i’ is immutable, so it is safe to return it instead of a copy. xyI}y(CN1
return i; /7gOSwY
} q$=#A7H>3)
(<^ yqH?
w*R$o
8By|@LO
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: eq UME
h:9Zt0,
#8)*1?
;Iq/l%vX
?Boolean l+V>]?j
?Byte K4kMM*D
?Character ,G)r=$XU
?Class T#>7ub
?Double ocs+d\
?Float K'GBMnjD
?Integer /~3r;M
?Long H)n9O/u
?Short aA,!<^&}
?String K.0:C`C
?大部分的Exception的子类 Hw4%uS==V
1YH+d0UGn
MG.`
r{5
w!D|]LoE
四、常见错误4# :自编代码来拷贝数组 55z]&5N
9Q"'"b*?z
>3Eo@J,?d
I"GB<oB
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: EVGt 5z
+llR204
!jTcsN%
Y=Kc'x[,Zj
public class Example{ 8SGo9[U2
private int[] copy; &G-!qxe
/*** Save a copy of ’data’. ’data’ cannot be null.*/ xoN3
public void saveCopy (int[] data){ i*Z"Me
copy = new int[data.length]; .hifsB~
for (int i = 0; i < copy.length; ++i) Om5Y|v"*
copy = data; .4E&/w+
} 3U0`,c\ao*
} BBev<
?U2<
9?SZNL['V
U[ 0=L`0e
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: va0{>Dc+
jEZMUqGY!
6}!#;@D~
Eqj_m|@
void saveCopy (int[] data){ rogT~G}q
try{ H*r)Z90
copy = (int[])data.clone(); 4GX-ma,
}catch (CloneNotSupportedException e){ B\o Mn
// Can’t get here. C)`Fv=]R
} 85LAYaw
} z62;cv
j3{D^|0bP
yjF1}SQ
7Mg=b%IYs
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: ci?qT,&
0|{u{w@!`
@fl-3q
~
Q. 7VDz
static int[] cloneArray (int[] data){ 'ZDp5pCC;
try{ gMzcTmbc8
return(int[])data.clone(); x$6^R q>2
}catch(CloneNotSupportedException e){ F9,DrB,B{
// Can’t get here. ,Y/ g2
4R
} !:q/Ye3.
} ,X`)ct
6">+
~
G
,g2ij
e,W%uH>X
这样的话,我们的saveCopy看起来就更简洁了: NTYg[VTr
%H]ptH5
ur:3W6ZKl
5\]Sv]s)R
void saveCopy (int[] data){ pHLB = r
copy = cloneArray ( data); hEKf6#
} Z{]0jhUyNh
7$CBx/X50)
HTX?,C_
Brf5dT49
五、常见错误5#:拷贝错误的数据 v|dBSX9k0
6WXRP;!Q
CxwoBuG=?
`erV$( M
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: /`wvxKX
Y 0d<~*
@~^5l
TFlet"ge=
import java.awt.Dimension; j+$rj
/*** Example class. The height and width values should never * be ]:XoRyIZ1[
negative. */ ,$s8GAmq
public class Example{ n\*!CXc
static final public int TOTAL_VALUES = 10; |)(VsVG&
private Dimension[] d = new Dimension[TOTAL_VALUES]; Egg=yF>T
public Example (){ } X= 5xh
u)}$~E>
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ UC]\yUK1J
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ 0IBhb(X
if (height < 0 || width < 0) Lr$go6s
throw new IllegalArgumentException(); dfKF%27
if (d[index] == null) ,!#*GZ.ix
d[index] = new Dimension(); C~2F9Pg
d[index].height = height; haK3?A,"_A
d[index].width = width; gG<~-8uQ
} M2OIBH4!
public synchronized Dimension[] getValues() _>(^tCo
throws CloneNotSupportedException{ <>y;.@}Q
return (Dimension[])d.clone(); itBwCIj G
} -GhP9; d
} [q?<Qe
,|y:" s
WrQD X3
hI]Hp3S
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: B-ngn{Yc
^o3"#r{:+
Ve}(s?hU5
_(%d(E2?
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ <D<4BnZ(
Dimension[] copy = (Dimension[])d.clone(); "p_J8
for (int i = 0; i < copy.length; ++i){
P5a4ze
// NOTE: Dimension isn’t cloneable. 8 OY 3A
if (d != null) ,?8qpEG~#+
copy = new Dimension (d.height, d.width); x@P y>f2
} $PTP/^
return copy; m0ER@BXRn
} {o_X`rgrL
!h"Kq>9T
,J,/."Y
1+szG1U=
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: =RA /
DS+}UO
:ubV };
4>F'oqFF
public void store (int[] data) throws CloneNotSupportedException{ 0m%|U'm|j
this.data = (int[])data.clone(); gd%NkxmW
// OK q)X$^oE!6
} <"{qk2LS1
Uzz'.K(Mv|
rI= v
be]bZ
1f
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: Tl(^
F,W~,y
27
]':A4_
TSTl+W
public void wrongStore (int[][] data) throws CloneNotSupportedException{ ]zj9A]i:a
this.data = (int[][])data.clone(); // Not OK! R "n5
} ^U
`[(kz=
public void rightStore (int[][] data){ Ixb=L(V
// OK! q)LMm7
this.data = (int[][])data.clone(); :o0JY= 5
for (int i = 0; i < data.length; ++i){ ;&<{ey
if (data != null) "?]{%-u
this.data = (int[])data.clone(); iHeN9 cl
} z:8eEq3w
} c`J.Tm[_u
<sWprR
h1B? 8pD
qaiNz S@q
&+Z,hs9%
六、常见错误6#:检查new 操作的结果是否为null 6h|q'.Y
z.7cy@N6
f[<m<I
B:5Rr}eY+
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: )WRLBFi3
"'c
A2~
X
iS1\*
G,?hp>lj
Integer i = new Integer (400); QQ%D8$k"
if (i == null) ]RPs|R?
throw new NullPointerException(); 10)jsA
mw_~*Nc'9
5's87Z;6
XC4X-j3
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 l)G^cSHF.3
>p)MawT]
l1T m`7}
g[1gF&
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 .<jr0,i
YPU*@l>
七、常见错误7#:用== 替代.equals 5:pM4J
QKyo`g7
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: pf1BN@
t
U &C!}
VPO
N-{=`
C"6?bg5N
int x = 4; kE:nsXI
)
int y = 5; < Wfx+F
if (x == y) x.7]/)
System.out.println ("Hi"); ;XF:\<+
// This ’if’ test won’t compile. cJ{ Nh;"
if (x.equals (y)) I;e=0!9U
System.out.println ("Hi"); \n$u)Xj~6^
h]Wr [v
4lr(,nPRD
T=iJGRctB
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 HBy[FYa4
=Q+;=-1
NG--6\
2;zb\d
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 _2hS";K
SG6kud\b
H<VTa? n
_y),J'W^3u
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 tz5e"+Tz
W=j[V
Oq
\#f<!R4
ZIf
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 5*j?E
/I1h2E
0rOfrTNOz%
)k\H@Dy%$
八、常见错误8#: 混淆原子操作和非原子操作 +1uF !G&l
U}6B*Xx'
6ys
&zy
]"t@-PFX<
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: qAAX;N
!ipR$ dM
\?Z{hmN
W }8'Pf
public class Example{ qlb-
jL
private int value; // More code here... 4.Q} 1%ZN
public void set (int x){ @aAW*D~-J
// NOTE: No synchronized keyword |%J {RA
this.value = x; -7*ET3NSI/
} v/](yT
} [Yo,*,y31
brW :C?}
{1IfU
ZX>AE3wk
不过,这个保证仅限于读和写,下面的代码不是线程安全的: S4'
T;L>;E>B
(MR_^t
zfc'=ODX
public void increment (){ SW*"\X;
// This is effectively two or three instructions: : ]sUpO
// 1) Read current setting of ’value’. $K]m{
// 2) Increment that setting. Z1 Bp+a3
// 3) Write the new setting back. 6A>dhU
++this.value; <QA6/Ef7
} 8kU!8^mH
C"!gZ8*\!9
o9JMH.G
v*;-yG&
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: ex::m&
P,xKZ{(
+_; l|uhT;
-n=^U
public synchronized void increment (){ Ont%eC\
++this.value; /JHc! D
} J&M
o%"[)
7[> 6i
b\3Oyp>
?98("T|y;
九、常见错误9#:在catch 块中作清除工作 ~rDZ?~%
lwrCpD.
,quoRan
L;*ljZ^c
一段在catch块中作清除工作的代码如下所示: gu0j.XS^
\9cG36
6G
#}Q/
:+qF8t[L
OutputStream os = null; l5zS
try{ *A"~m!=
os = new OutputStream (); {U1?Et#
// Do something with os here. Oy%''+g
os.close(); M-1ngI0H;
}catch (Exception e){ fz\9 S
if (os != null) t"=
E^r
os.close(); 2nSSFx r
} >33=<~#n
|$vX<. S
{[+mpKq
v hpNpgz
尽管这段代码在几个方面都是有问题的,但是在测试中很容易漏掉这个错误。下面列出了这段代码所存在的三个问题: Kla'lCZ
$6mX
cki81bOT
\HKxh:F'
1.语句os.close()在两处出现,多此一举,而且会带来维护方面的麻烦。 YL]Z<%aKt
|G?htZF
Y8m1M-#w
.#rJ+.2
2.上面的代码仅仅处理了Exception,而没有涉及到Error。但是当try块运行出现了Error,流也应该被关闭。 `(YxI
umiBj)r
E%rk[wI
;$smH=I
3.close()可能会抛出异常。 d8[J@M53|T
,q}MLTSi
H@q?v+2
U*22h` S
上面代码的一个更优版本为: ujlY!-GM
_H j!2 '
;_rF;9z9
\Ta"}TF8
OutputStream os = null; ldiD2
Q
try{ Fs9I7~L3
os = new OutputStream (); syaPpM
Q-
// Do something with os here. nm6h%}xND<
}finally{ ~]nSSD)\
if (os != null) v^#~98g]
os.close(); j`~Ms>
} kQEy#JQmB
tasUZ#\6
f@Zszt
Q36qIq_0e
这个版本消除了上面所提到的两个问题:代码不再重复,Error也可以被正确处理了。但是没有好的方法来处理第三个问题,也许最好的方法是把close()语句单独放在一个try/catch块中。 V:VO[e<e
~GL]wF2#
n ~shK<!C
-'t)=YJ
十、常见错误10#: 增加不必要的catch 块 2/"u5
IIn"=g=9
G/7cK\^u
IOqwCD[
一些开发者听到try/catch块这个名字后,就会想当然的以为所有的try块必须要有与之匹配的catch块。 uI1q>[
XCU7xi$d
w8U&ls