代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 ppO!v?
c32"$g
bLUyZ3m!
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 <O{G&
6lwWFR+k
VGOdJ|2]Wr
8,:lw3x1
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 Gn<e&|4>i}
pzU:AUW
'JAe=K
H
l#]+I YD
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 pH0MVu(W
v&` n}lS
^{-Z3Yxd
&p=(0$0&-
一、常见错误1# :多次拷贝字符串 +lJD7=%K]Z
DMT2~mh
5gwEr170
) 3I|6iS
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 YV6w}b:
kb'l@d#E
D
\boF+^
dkZ[~hEQG-
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: Rtai?
}$:ha>
EtDzmpJR>
O! w&3 p
String s = new String ("Text here"); ?$b*)<
7[8d-Sf24{
g]._J
5~"m$/yE
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: P2 +^7x?
xic&m5j
m
Q5;EQ.#
?<soX8_1
String temp = "Text here"; L(BL_
String s = new String (temp); AUR{O
5ma~Pjt8}
hy@e(k|S]U
>
Cx;h=
但是这段代码包含额外的String,并非完全必要。更好的代码为: _Tf0L<A'R
q_:B=w+bC
-J++b2R\%
EyV6uk~
String s = "Text here"; 1(4IcIR5T;
N'8}5Kx5
))uki*UNK
8FBXdk?A
二、常见错误2#: 没有克隆(clone)返回的对象 wQX%*GbL2
0f,Ii_k bT
<:~'s]`zf
d'p@[1/
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: nAyyjd3!S
lUHpGr|U%
E\~!E20^
0j[%L!hny
import java.awt.Dimension; }vQY+O
/***Example class.The x and y values should never*be negative.*/ R<ZyP~
public class Example{ HuajdC~
private Dimension d = new Dimension (0, 0); 1!2,K ot
public Example (){ } mQ:5(]v
T?8N$J
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ pg4jPuCM
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ 1Gk'f?dw
if (height < 0 || width < 0) lLuAg ds`
throw new IllegalArgumentException(); n}q/:|c
d.height = height; X6o
iOs
d.width = width; ['@R]Si"!
} efm#:>H
Qs\!Kk@
public synchronized Dimension getValues(){ [\)irCDv
// Ooops! Breaks encapsulation gOn^}%4.I
return d; (%|L23
} 8MCSU'uQ
} OyTp^W`&
<{A |Xs
I)f54AX
gK-$y9]~+
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: P\.1w>X
gz`P~7-w:
!T26#>mV
1&JB@F9!
Example ex = new Example(); _6MNEoy?
Dimension d = ex.getValues(); _<;westq
d.height = -5; {@3p^b*E)1
d.width = -10; 8Sg:HU\
WJw
%[_W
*Duxabo?
-wn(J5NnR
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 )R"deb=s
!8OUH6{2
YX6[m6LU
F$>^pw
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 RyN?Sn5)
;NrU|g/ksX
l|~SVk|
x-ZCaa}O
更好的方式是让getValues()返回拷贝: c!>",rce
T\$r|
oA$]%
Ed&M
public synchronized Dimension getValues(){ #wZBWTj.
return new Dimension (d.x, d.y); J l9w/T
} p+|(lrYC
jRo4+8
xouy|Nn'
<LOas$
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 9/R<,
}TAHVcX*p
naWW i]9
zrCQEQq
三、常见错误3#:不必要的克隆 gAViwy9{
ttC+`0+H
~:lN("9OI
}e0)=*;l
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: Zk75GC
,[0rh%%j
eXZH#K7S#
A;#GU`
/*** Example class.The value should never * be negative.*/ $sR-J'EE!
public class Example{ 00(#_($
private Integer i = new Integer (0); 5_ioJ
public Example (){ } #u6ZCv7u
XveG#oyiU
/*** Set x. x must be nonnegative* or an exception will be thrown*/ 6?(vXPpT$
public synchronized void setValues (int x) throws IllegalArgumentException{ \Dn
an5H/
if (x < 0) NHq*&xy
throw new IllegalArgumentException(); 5qx$=6PT
i = new Integer (x); [}!obbM
} h>A}vI*:
c<j+"
public synchronized Integer getValue(){ .jjvS
// We can’t clone Integers so we makea copy this way. !aub@wH3
return new Integer (i.intValue()); Cz1o@rt
} %O_Ed
{G4t
} N8w@8|KM
w0N8a%
e4?p(F-x(
]
cY
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 X7gtR|[
J`x!c9 zg7
t|y`Bl2
$6p|}<u
方法getValue()应该被写为: B\}B
H
5(sWV:_2
V;-YM W
gzDNMM
public synchronized Integer getValue(){ @G;\gJT*
// ’i’ is immutable, so it is safe to return it instead of a copy. 2
.)`8|c9
return i; |=9=a@l]P
} ^%r>f@h!L
=jN9PzLk
WGrG#Kw[
z^r
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: ~}fQ.F*7R
q-)Ynp4'
c-{;P>L
`;fk,\8t%
?Boolean P_f^gB7
?Byte | &]04
?Character my^2}>wi
?Class 5U+a{oA
?Double XKq}^M&gy
?Float <X,0\U!lL
?Integer 8~")9w
?Long R7xEE7p
?Short J|A:C[7 2
?String Y"&1jud4xl
?大部分的Exception的子类 hB)TH'R{:
M}
{'kK
3\jcq@N
+P. }<
四、常见错误4# :自编代码来拷贝数组 ayvHS&h
8
k%!1dyMB
g`BtG
)+S^{tt
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: ~qxuD_
"dO>P*k,
Hkck=@>8H*
rFPfTpS
public class Example{ \h}a?T6
private int[] copy; 2'6:fr=R
/*** Save a copy of ’data’. ’data’ cannot be null.*/ )HN,A z"
public void saveCopy (int[] data){ ] oh.w
copy = new int[data.length];
xfyUT^
for (int i = 0; i < copy.length; ++i) ?QXc,*=N
copy = data; O~WT$
} ;=[~2*8
} &:"[hU
m|5yET
bez_|fY{T
$WV N4fg
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: ]7ZY|fP2
c<gvUVHIxR
_PR><L_
OAhCW*B
void saveCopy (int[] data){ bq<DW/
try{ >x$.mXX{
copy = (int[])data.clone(); f*}H4H E O
}catch (CloneNotSupportedException e){ jZ8#86/#{
// Can’t get here. 1hQeuG
} tb@&!a$`?
} .;&1"b8G
psHW(Z8G
oMj;9,WK'
JNYFu0
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: 5#SD$^
I2$.o0=3Y
e+t2F
|xDh
p+F{iMC
static int[] cloneArray (int[] data){ s}pn5zMp:8
try{ ,?Bo
x
return(int[])data.clone(); ~A5MzrvIO2
}catch(CloneNotSupportedException e){ s$s]D\N
// Can’t get here. lB0: 4cIj
} .jfkOt?2
} _
IqUp Y
Jn>6y:s
Jt3]'Nr04@
c88I"5@[bD
这样的话,我们的saveCopy看起来就更简洁了: $O/@bh1@p
%;Dp~T`0
7Q(5Nlfcz
7Q>*]
void saveCopy (int[] data){ )Bq~1M 2
copy = cloneArray ( data); smM*HDK
} C)r!;u)AZH
w/`I2uYu
-m.SN>V
f;k'dqlv
五、常见错误5#:拷贝错误的数据 >%~%O`+
*Hnk,?kPq
FYe(SV(9
k>8,/ AZd
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: `n#
{} %
zMUifMiAj
_p~lL<q-K[
JY|f zL
import java.awt.Dimension; ];.H]TIc6
/*** Example class. The height and width values should never * be Xy>+r[$D:
negative. */
'7!b#if
public class Example{ D-[`wCa,
static final public int TOTAL_VALUES = 10; O<1qU
M
private Dimension[] d = new Dimension[TOTAL_VALUES]; V_&>0P{q
public Example (){ } X$L9kZ
\Ami-<T
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ MMpGI^x!-X
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ XkWO -L
if (height < 0 || width < 0) 0t-!6
throw new IllegalArgumentException(); @@,l0/
if (d[index] == null) 1HF=,K+
d[index] = new Dimension(); g?'4G$M
d[index].height = height; c:/H}2/C
d[index].width = width; bk**% ]
} [_&\wHX
public synchronized Dimension[] getValues() )PRyDC-
throws CloneNotSupportedException{ [HKTXF{n
return (Dimension[])d.clone(); !NNq( t
} zF6]2Y?k%
} RN0@Q~oTI
tntQO!pM
Wb_'X |"u
}=':)?'-.
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: O43emL3
[f'V pId8
;$Eg4uX
p?mQ\O8F
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ ZYy,gu<
Dimension[] copy = (Dimension[])d.clone(); &uh|!lD
for (int i = 0; i < copy.length; ++i){ C0xjM0
// NOTE: Dimension isn’t cloneable. ]*8K4n G
if (d != null) jXBAo
copy = new Dimension (d.height, d.width); Ylc[ghx
} p nS{W
\Q
return copy; e5!LbsJv
} IpX>G]"-C
m"{D}(TA
CH6^;.
fa7I6 i
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: Pd99vq/
w&eX)!
vjy 59m
yw|O,V<4N
public void store (int[] data) throws CloneNotSupportedException{ 9'tElpDJ6#
this.data = (int[])data.clone(); 0-ISOA&
// OK vI<n~FHt
} >a@c5
9oly=&lJ
<q
V<dK&W
28KS*5S
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: a9CY,+z5B
XwKB+Yj0
[
7W@/qqv
Iv6(Z>pAB
public void wrongStore (int[][] data) throws CloneNotSupportedException{ os<B}D[
this.data = (int[][])data.clone(); // Not OK! @z8,XW
}
} wHSa s[4k
public void rightStore (int[][] data){ l-Hp^|3Wq
// OK! ggr\nY
this.data = (int[][])data.clone(); PVGvj c
for (int i = 0; i < data.length; ++i){ pDGX$1O"
if (data != null) X>Cl{.
this.data = (int[])data.clone(); B|Y6;4?
} (mHCK5
} 481SDG[b
dqU
bJc]
?mdgY1
0XCtw6
$
e<&7
六、常见错误6#:检查new 操作的结果是否为null iez@j
-^m]Tb<u
29(s^#e8A
q[l!kC+Eh
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: \,<5U
F0
zJnF#G
0v%ZKvSID
$"z|^ze
Integer i = new Integer (400); <IWO:7*#
if (i == null) I:4m]q b
throw new NullPointerException(); $F|3VQ~
xEt".K
={[s)G
VKcO]_W1
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 Mqu>#lL
q*,g
"0JG96&\
FL}k0
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 (Sr D
#7!P3j
七、常见错误7#:用== 替代.equals o1x IGP<
^l}Esz`-M
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: ys+ AY^/
]?n)!u
xh$1Rwa
{\!_S+}{
int x = 4; 3urL*Fw,
int y = 5; %:bTOw[4r
if (x == y) ][b_l(r$?
System.out.println ("Hi"); !a"RHg:HO
// This ’if’ test won’t compile. s8,N9o[.~P
if (x.equals (y)) DYS|"tSk
System.out.println ("Hi"); l+e L:C!
s68&AB
%E\&9,
L0\97AF
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。
0G-M.s}A
95Q{d'&
da c?b(
[D[&aA
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 Z^AOV:|m
q.s 2x0
~f/nq/8
cVHv>nd#
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 =.q
Zgcg
$i s|B9B
JZQT}
Rj&V~or
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 g. V6:>,
)sWC5\
FyZp,uD
mTG v*=l
八、常见错误8#: 混淆原子操作和非原子操作 n9.` 5BH7/
;J"b% ~Gn
,L iX
de.!~%D
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: %kM|Hk3d
[i7Ug.Oi"
L
B:wo.X
U#=Q`
public class Example{ $vlc@]~d`&
private int value; // More code here... ghXh nxG
public void set (int x){ $tGk,.#j
// NOTE: No synchronized keyword u`Ew^-">
this.value = x; 2=X\G~a
} ?NV3]vl
} 4%h@K(iN
qT(
3M9!
}Wxu =b
<t9#~x#'b
不过,这个保证仅限于读和写,下面的代码不是线程安全的: %_*q'6K
`4Jlf!
*],]E;
wYTF:Ou^5~
public void increment (){ 7O3 \
// This is effectively two or three instructions: a78&<
// 1) Read current setting of ’value’. -p|@En n
// 2) Increment that setting. 577H{;pW
// 3) Write the new setting back. /ESmQc:DWB
++this.value; yFp8 >
} KMsm2~P
?eUhHKS5
aE0yO#=
Iu`B7UOF
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: F1s kI _!
&5Ai&<q"p
/IDfGAE
XWQp-H.
public synchronized void increment (){ joa|5v'
++this.value; :b^\O
} ]YF[W`2h
aBX^Wd
Y<X,(\iEHP
y}NBJ
九、常见错误9#:在catch 块中作清除工作 bAIo5lr
+" 4E:9P?
GT|=Kx$;
f_}FYeg
一段在catch块中作清除工作的代码如下所示: =Z
^=
QO;W}c:N
V\nQHzjF<6
-3 }
OutputStream os = null; +we3BE.
try{ p9*#{~
os = new OutputStream (); k(>hboR5n
// Do something with os here. !b<c*J?f
os.close(); !o.l:Mr
}catch (Exception e){ *M*:3v
0
if (os != null) vO#4$,
os.close(); !MNo
8dC;
} gie}k)&M
)L?JH?$C
T7E9l
Q<$I,C]
尽管这段代码在几个方面都是有问题的,但是在测试中很容易漏掉这个错误。下面列出了这段代码所存在的三个问题: S:qML]RO
_9!_fIY
Xz`?b4i
=y"
lX{}G
1.语句os.close()在两处出现,多此一举,而且会带来维护方面的麻烦。 @}&o(q1M0
B|#*I[4`w@
Hd(|fc{2
MqXN,n+`k
2.上面的代码仅仅处理了Exception,而没有涉及到Error。但是当try块运行出现了Error,流也应该被关闭。 D4?qw$"
k8E'wN
(dO, +~
bg$df 0
3.close()可能会抛出异常。 `.PZx%=
ax7]>Z=%d"
N~H9|CX
r0=Aru5n
上面代码的一个更优版本为: T9enyYt%
OyVdQ".
1-C 2Y`
KL]@y!QU
OutputStream os = null; d,j"8\@
try{ |ToCRM
os = new OutputStream (); A!}Wpw%(/
// Do something with os here.
:~JgB
}finally{ e6{}hiM
if (os != null) 1X\dH<B}
os.close(); J[fjl6p
} FilHpnQCt
W.h6g8|wx
X^4HYm
=8`,,=P^
这个版本消除了上面所提到的两个问题:代码不再重复,Error也可以被正确处理了。但是没有好的方法来处理第三个问题,也许最好的方法是把close()语句单独放在一个try/catch块中。 hz8Y2Ew
>/;V_(
N_TWT&o4
9kj71Jp&}
十、常见错误10#: 增加不必要的catch 块 4}sfJ0HhX
wkm;yCF+
SEm3T4dfzf
,ZyTYD|7
一些开发者听到try/catch块这个名字后,就会想当然的以为所有的try块必须要有与之匹配的catch块。 aML?$_6
`A O_e4D0i
:Mr _/t2(
xk=5q|u_-
C++程序员尤其是会这样想,因为在C++中不存在finally块的概念,而且try块存在的唯一理由只不过是为了与catch块相配对。 r=[T5,L(s
e2|2$|
f1F#U@U
$5aRu,
增加不必要的catch块的代码就象下面的样子,捕获到的异常又立即被抛出: \gferWm
TqK`X#Zq
Tvr2K84l
{f]K3V
try{ O:'UsI1Y
// Nifty code here j`1%a]Bwc
}catch(Exception e){ kmjSSh/t
throw e; &i*/}OZz
}finally{ @K`2y'#b
// Cleanup code here GD?4/HkF
} 9(k5Irv"'h
]8*#%^
XiE
d0YN:lJc
不必要的catch块被删除后,上面的代码就缩短为: ~0 <?^
?<c)r~9]
Y9fktg.
#N\kMJl$l
try{ LU5e!bP
// Nifty code here !MoJb#B3^]
}finally{ t-gg,ttnA
// Cleanup code here p
b:mw$XQ7
} YX38*Ml+V
dXgj
ur^)bp<