代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 %>y!N!.F
s5MG#M 9
|I|,6*)xg
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 h96<9L
k RSY;V
qP=a:R-
0TiDQ4}i[
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 ?,[$8V
AU9:Gu@M/
yHZ&5
*GdJ<B$
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 NvpDi&i
dKw[#(m5v
c^5fhmlt
]J/;Xp
一、常见错误1# :多次拷贝字符串 CL t(_!q
)]m4FC:
z@jKzyq
e%0IEX
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 &iy7It
m]{/5L
8%MF<
?4R q +
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: X6?Gxf,
/~?[70B}E
" ;\EU4R
VHL[Y
String s = new String ("Text here"); hWX4 P
a"k,x-EL(
!wro7ilMB
X-j<fX_
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: vQDR;T"]
ye| 2gH
%fh-x(4v
5yroi@KT
String temp = "Text here"; +>%AG&Pc
String s = new String (temp); s:tWEgZk?
;\T~Hc}&;
5?E;YyA
Bf W@f
但是这段代码包含额外的String,并非完全必要。更好的代码为: =knBwjeD
UK[+I]I
p
QX.U:p5C
~6OdwGWV
String s = "Text here"; (W9 K:]}
|%(qaPA1
mDWRYIuN
XKT[8o<L
二、常见错误2#: 没有克隆(clone)返回的对象 X8XE_VtP
^4{{ +G)j
>vuR:4B
]:4\rBR3
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: 9*CRMkPrd
8BnsYy)j
YsRq.9Mr
/T 4GPi\lg
import java.awt.Dimension; VB4ir\nF
/***Example class.The x and y values should never*be negative.*/ t & 5s.
public class Example{ h>/L4j*Z
private Dimension d = new Dimension (0, 0); N,ZmGzNP)
public Example (){ } Mo4igP
mDA1$fj"
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ }O6E5YCm
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ 9;A9Q9Yr
if (height < 0 || width < 0) !1bATO:x
throw new IllegalArgumentException(); +1Rz +
d.height = height; e&9v`8}
d.width = width; !@
)JqF.
} 2W)KfS
h<BTu7a`r
public synchronized Dimension getValues(){ -TyBb]
// Ooops! Breaks encapsulation {ka={7
return d; YXGxE&!
} 1(Lq9hs`
} /8lmNA
`>k7^!Ds
P0-K/_g
\Iz-<:gA'
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: F=;nWQ&
_P=L| U#C
QU@CPME
-Z:nImqzc
Example ex = new Example(); ,k,+UisG
Dimension d = ex.getValues(); LlbE]_Z!U%
d.height = -5; VS5D)5w#
d.width = -10; Pm|S>r
NF_[q(k'
2K{)8;^
!LpFK0rw
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 ,.uI>
.gw6W0\F
8oP"?ew#
x\5\KGw16
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 QV=|'
S
<T$rvS
en16hd>^W:
<!~NG3KW[>
更好的方式是让getValues()返回拷贝: &3YXDNm
rmhL|!
Y
ZV~9{E8
d-#yN:}0
public synchronized Dimension getValues(){ s&-dLkis{u
return new Dimension (d.x, d.y); VCUsvhI
} AH#Dk5#G
(KphAA8
*Di ;Gf@
B|-W
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 8?t}S2n2
%r:Uff@
}<H0CcG
= /=?l
三、常见错误3#:不必要的克隆 /6#i$\ j
2S-z$Bi}]
h
x
hl
?"T *{8
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: x)e(g}n
Xxs0N_va&
b|g=&T:pp
r} a,
/*** Example class.The value should never * be negative.*/ +J:wAmY4
public class Example{ z;EDyd,O>
private Integer i = new Integer (0); 5f_1 dn
public Example (){ } ??g
=
`yH
]goPjfWvU"
/*** Set x. x must be nonnegative* or an exception will be thrown*/ /Au7X'}
public synchronized void setValues (int x) throws IllegalArgumentException{ 3>k?-%"
if (x < 0) /m+.5Qz9)@
throw new IllegalArgumentException(); dqw0ns.2
i = new Integer (x); mUwGr_)wj
} X%Ta?(9|.^
!]!J"!xg*
public synchronized Integer getValue(){ Qy|6A@
// We can’t clone Integers so we makea copy this way. u S{WeL6%
return new Integer (i.intValue()); c4FU@^Vv
} Q%_MO`<]$
} O3(H_(P
M[Mx
g
3aEO9v,n
RVa{%
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 + :k"{I
X5o{d4R L
!+ ??3-q
$Q*^c"&
方法getValue()应该被写为: Yj>ezFo
#:}mi;{
3>buZ6vh
O}q(2[*i
public synchronized Integer getValue(){ =42NQ{%@;
// ’i’ is immutable, so it is safe to return it instead of a copy. k3r<']S^
return i; qJEtB;J'
} 420yaw/":
,M$J
yda
8jfEvwY
MD=!a5'
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: +&8'@v$
&W-1W99auE
, @dhJ8/
!_j6\r=
?Boolean 2"L a}Vx2
?Byte >Ms_bfSK
?Character 8Z(\iZ5Rgj
?Class YEhPAQNj
?Double F=~LVaF/_
?Float 6B`,^8Lp
?Integer Rln@9muXA
?Long $1;@@LSw
?Short yo"!C?82=
?String F]`_ak E
?大部分的Exception的子类 (j884bu
*|&&3&7
vB1nj<]&z
K.: :P84m;
四、常见错误4# :自编代码来拷贝数组 #]FJx
be:=-B7!
i$S*5+
Kma-W{vGD
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: ;@G5s+<l
&Vmx<w
2N}h<Yd9
+pJ~<ug]
public class Example{ udGZ%Mr_
private int[] copy; qq[Enf|/y
/*** Save a copy of ’data’. ’data’ cannot be null.*/ Ai.^~#%X
public void saveCopy (int[] data){ Bz*6M
copy = new int[data.length]; T{mIkp<
for (int i = 0; i < copy.length; ++i) "y$s`n4Mj
copy = data; d m$iiRY
} [rtMx8T
} k|[86<&[
geEETb}+y
>'2=3L^Q
7DCu#Y[
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: WS1$cAD2N
I vD M2q8f
]ppws3*Pa
()%;s2>F
void saveCopy (int[] data){ &(,-:"{pNR
try{ *4RL
copy = (int[])data.clone(); Xrd-/('2
}catch (CloneNotSupportedException e){ T96M=?wh!
// Can’t get here. _"'0^F$I
} o&U/e\zy
} $JZ}=\n7
!t+eJj
RL~]mI!U
6SN$El 0|G
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: x] j&Knli
LCkaSv/[RB
\s">trXwX
W#lt_2!j
static int[] cloneArray (int[] data){ fW8whN
try{ <-Q0s%mNj,
return(int[])data.clone(); :}p<Hq 8Z
}catch(CloneNotSupportedException e){ +U*:WKdI?
// Can’t get here. >q)VHV9P
} p28=l5y+
} g"Gj8QLDz
|aMeh;X t
`w/b];e1)
]sG^a7Z.X
这样的话,我们的saveCopy看起来就更简洁了: |^$?9Dn9.L
P_Ni
5s)
BewJ!,A!
k#pNk7;MZ
void saveCopy (int[] data){ *-.,QpgTX
copy = cloneArray ( data); 7)37AK w
} S7WT`2
$ J)2E g
O>kM2xw
0rj50$~$]
五、常见错误5#:拷贝错误的数据 Xhm)K3RA*T
RoeLf Ow
pQ yH`
R1NwtnS
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: GP;UuQz
&1$|KbmV4
a7wc>@9Q,
U#
7K^(E9
import java.awt.Dimension; d0 qc%.s
/*** Example class. The height and width values should never * be ^A' Bghy
negative. */ ;J&9l
>
public class Example{ <A@qN95m
static final public int TOTAL_VALUES = 10; .YxcXe3#
private Dimension[] d = new Dimension[TOTAL_VALUES]; a5@XD_b
public Example (){ } ;iTZzmB
);oE^3]f
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ *ci%c^}V
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ d td}P~
if (height < 0 || width < 0) fi;00>y
throw new IllegalArgumentException(); Tg\wBhJr|
if (d[index] == null) %:/?eZ
d[index] = new Dimension(); 1@{qPmf^
d[index].height = height; J!@`tR-
d[index].width = width; 4+'d">+|
} u:GDM
public synchronized Dimension[] getValues() 6R+EG{`
throws CloneNotSupportedException{ wTkcR^
return (Dimension[])d.clone(); HA0Rv#p
} *zTEK:+_
} SWPb=[WEz
{axMS yp;
G+zIh}9
FCA]zR1
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: 2}jC%jR2
xI(Y}>
Yo;Mexo!
l~c# X3E
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ pIP^/H
Dimension[] copy = (Dimension[])d.clone(); N@G~+GCxL
for (int i = 0; i < copy.length; ++i){ (7J (.EG2e
// NOTE: Dimension isn’t cloneable. G*\U'w4w|*
if (d != null) /j:fc?yv
copy = new Dimension (d.height, d.width); wC~LZSTt
} ]0@
06G(y
return copy; lz88//@gZ
} b?deZ2"L#
.U9A\$
ePxwN?
.}x:yKyi@
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: P2>Y0"bY
\YrvH
3}ATt".
4VrL@c
@
public void store (int[] data) throws CloneNotSupportedException{ P[<EFjE
this.data = (int[])data.clone(); &