代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 g;w4:k)U
bmna*!l^M
V|
z|H$-
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 3JEH
sYxs
ya{vR*
'~
MzYTEe&-L
K$(&Qx}
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 Z'<=06
^*'|(Cv
j#y_#
?I)-ez
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 ~|@ aV:k
gt6*x=RCrQ
\ntmD?kA
)ruC_)
一、常见错误1# :多次拷贝字符串 N:[m,U9a
}Y[Z`w
'(Uyju=
zMt "ST.
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 g"(
vl-Uw
Y'S xehx
?mS798=f
4JFi|oK0H
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: &M=12>ah]
fG;)wQJ
o %A4wEye
lYT}Nc4"="
String s = new String ("Text here"); CjORL'3
:2Qm*Y&_$V
`rW{zQYM
:+ @-F>Q
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: r0l ud&_9
b|n%l5
1
}b2U o&][
-w=rNlj
String temp = "Text here"; *fW&-ic
String s = new String (temp); IyIh0B~i
"2+>!G RQ
PHi'&)|
lF5;Kc
但是这段代码包含额外的String,并非完全必要。更好的代码为: Bo.x
xT{qeHeZ9,
)QaI{ z
-'3vQXj&
String s = "Text here"; #B"ki{Se*
COc1np
W!.UMmw`
^i&/k
二、常见错误2#: 没有克隆(clone)返回的对象 rw8O<No4.o
{o+aEMhM
PV(bJ7&R
9fMg?
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: jpZX5_o
9z\q_0&i
< Upn~tH
511^f`P<
import java.awt.Dimension; kf_s.Dedw
/***Example class.The x and y values should never*be negative.*/ ?,]%V1(@V`
public class Example{ 468LVe?0
private Dimension d = new Dimension (0, 0); 3l->$R]
public Example (){ } kI]i,v#F
5&v'aiWK
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ tz
j]c
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ 8|{:N>7
if (height < 0 || width < 0) X}0NeG^'O
throw new IllegalArgumentException(); X|L.fB=
d.height = height; `hM`bcS
d.width = width; ~^$ONmI5
} Thn-8DT
^=bJ
_'
public synchronized Dimension getValues(){ huWUd)Po%
// Ooops! Breaks encapsulation /8Bh
return d; jIv+=b#oT
} <tuh%k
} M3K+;-n^
R}llj$?
&\. LhOm
3ypB~bNw
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: Sq %BfP)a(
4?><x[l2{
&qz&@!`
?{\8!_Gvsl
Example ex = new Example(); u3Z*hs)Z%
Dimension d = ex.getValues(); s!nFc{
d.height = -5; /$\yAOA'y
d.width = -10; k )Z?
.sAcnf"
7.CzS
ipjl[
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 kGc;j8>."
SEr\ u#
2U2=ja9:Y
?'P8H^K6u
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 xE;4#+_I
D@^ r
%FT F
tNjb{(eO\h
更好的方式是让getValues()返回拷贝: 3F5r3T6j}
vUS$DUF
uZz^>*b
z[0L?~$
public synchronized Dimension getValues(){ 7SoxsT)
return new Dimension (d.x, d.y); 8UwL%"?YB
} `O.*qs5
FfI$3:9
m=z-}T5y!T
\! Os!s
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 DC]FY|ff
KqcelI?-I
+z+25qWi
^(V!vI*
三、常见错误3#:不必要的克隆 Yt++?
93kSBF#
#?}k0Y
[Rs5hO
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: j8M}*1
$Etf'.
RSG4A>%!mI
g (ZeGNV8
/*** Example class.The value should never * be negative.*/ ^>.?kh9z
public class Example{ t#&^ -;
private Integer i = new Integer (0); o(]kI?`
public Example (){ } }=^YLu=
~/!Zh
/*** Set x. x must be nonnegative* or an exception will be thrown*/ wHWd~K_q
public synchronized void setValues (int x) throws IllegalArgumentException{ 6JmS9ho
if (x < 0) WfhQi;r
throw new IllegalArgumentException();
0
!E* >
i = new Integer (x); Q pz01x
} <6mXlK3N0
:)g=AhBF
public synchronized Integer getValue(){ `R!0uRu
// We can’t clone Integers so we makea copy this way. r,2x?Qi
return new Integer (i.intValue()); ;s3"j~5m)
} -86 9$
} REW
*6:
{b<p~3%+Hc
9TO
2Q|Vg*x\U
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 3VCyq7B^
x7L$x=8s
YMIDV-
_;yp^^S
方法getValue()应该被写为: m qPWCFP
7{D+\i
o83HR[
P{)HXUVb
public synchronized Integer getValue(){ 5f=e
JDo=x
// ’i’ is immutable, so it is safe to return it instead of a copy. FxKH?Rl
return i; 7xVI,\qV
} bo$xonV @y
='pssdB
M86v
pA!+;Y!ZB<
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: |5F]y"Nb
[m|\N
rD%(*|Y"c
uCNQ.Nbf C
?Boolean !z{bqPlFGG
?Byte KB&t31aq
?Character @>qzRo
?Class LdU, 32
?Double wQ2'%T|t
?Float y
8];MTl
?Integer \$VtwVQ,b
?Long |C=^:@}ri?
?Short X3!btxa%t
?String bRLmJt98P
?大部分的Exception的子类 *Mg=IEu-6[
jzI\Q{[m'
,`P,))
3iV/7~
O
四、常见错误4# :自编代码来拷贝数组 W7l/{a
@
*VIM!/YW
e l'^9K
UI<'T3b
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: 6.EfM^[
d7It}7@9
Y_p
Z&s+*&TM
public class Example{ )!|K3%9
private int[] copy; w/d9S(
/*** Save a copy of ’data’. ’data’ cannot be null.*/ e|):%6#
public void saveCopy (int[] data){ 2~2
copy = new int[data.length]; @gE
+T37x2
for (int i = 0; i < copy.length; ++i) ok-sm~ bp
copy = data; n4>
} >`5iq.v
} n2Dnpe:
+_Fsiu_b
Q'*-gg&)
}}cVPB7
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: BtBy.bR
fk*JoR.o
>f'nl
^-~.L: }q
void saveCopy (int[] data){ .Ky<9h.K
try{ fT[6Cw5w`
copy = (int[])data.clone(); gO*cX&
}catch (CloneNotSupportedException e){ qnrf%rS
// Can’t get here. &I:X[=;g
} Gd%6lab
} 6\\B{%3R2
> :!faWX
lr +Kwve
$SG^, !!&A
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: qq[2h~6P]
}!Qo
wG
.3{S6#
d+fmVM?p
static int[] cloneArray (int[] data){ 70lb6A
try{ -66|Y
return(int[])data.clone(); #T#&qo#
}catch(CloneNotSupportedException e){ z.e%AcX
// Can’t get here. 1
YMaUyL
1
} &^ =t%A%#
} 0AJ6g@t[
asQ pVP
z ]o&^Q
: 60PO
这样的话,我们的saveCopy看起来就更简洁了: xb8fV*RO8A
}YU#}Ip@
X2dTV}~i
u-OwL1S+
void saveCopy (int[] data){ %+gze|J
copy = cloneArray ( data); {'"A hiR/
} KOhy)h+ h
fa\<![8LAU
6\4oHRJC
y\5V(Q\
五、常见错误5#:拷贝错误的数据 S,G=MI"
+_:Ih,-
0m7J'gm{
%[lX
H
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: r5lp<md
DXSZ#^,S[W
;NLL?6~
(z ;=3S
import java.awt.Dimension; <g>_#fz"K
/*** Example class. The height and width values should never * be 2?QIK3"v
negative. */ #Sb1oLC
public class Example{ v}xz`]MW<,
static final public int TOTAL_VALUES = 10; AJt0l|F
private Dimension[] d = new Dimension[TOTAL_VALUES]; y"e'Gg2
public Example (){ } 1'c!9
Y)c9]1qly
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ X]C-y,r[M
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ kul&m|
if (height < 0 || width < 0) ~;UK/OZ
throw new IllegalArgumentException(); )uwpeq$j7l
if (d[index] == null) {*
>$aI
d[index] = new Dimension(); ^5=}Y>EJO
d[index].height = height; 0J@)?,V-.
d[index].width = width; \ts:'
} G{+sC2
public synchronized Dimension[] getValues() =zqOkC
h$
throws CloneNotSupportedException{ PS`)6yn{_
return (Dimension[])d.clone(); ?h1]s&^|2
} hP3I_I[qF}
} a3HT1!M)
UgSSZ05Lq
W
qci51y>#
)P:TVe9`
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: B964#4&
9
TL]2{rf~
>/1.VT\E
f]T#q@|lE
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ IH}?CZ@{?
Dimension[] copy = (Dimension[])d.clone(); U>:CX
XHRt
for (int i = 0; i < copy.length; ++i){ `U2Z(9le
// NOTE: Dimension isn’t cloneable. ^B?{X|U37
if (d != null) |5e/ .T$
copy = new Dimension (d.height, d.width); -$dnUXFsj[
} NZ7a^xT_)
return copy; `+1*)bYxU
} f*W<N06EZ
l:j9lBS
D'Byl,W$
Uk|Xs~@#E
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: B`"-~4YAf
!x;T2l
+P}'2tE~'
hkHMBsNi
public void store (int[] data) throws CloneNotSupportedException{ :V}8a!3h
this.data = (int[])data.clone(); ,6i67!lb
// OK .s7o$u~l
} #(ANyU(#e
=ZzhH};aX
r^WO$u|@i
<X|"5/h
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: 2x$\vL0
~u,g5
g 4Vt"2|
1swh7
public void wrongStore (int[][] data) throws CloneNotSupportedException{ /~J#c=
this.data = (int[][])data.clone(); // Not OK! lNqXx{!k
}
S3)JEZi
public void rightStore (int[][] data){ 5T8X2fS:
// OK! 1tQZyHc42;
this.data = (int[][])data.clone(); #3kR}Amow
for (int i = 0; i < data.length; ++i){ 53BXz=
k
if (data != null) CM9+h;Zm
this.data = (int[])data.clone(); &>L\unS
} 'e;*V$+
} [A*vl9=
7lR(6ka&/
P1Re7/
EJdq"6S
3"I 1'+
六、常见错误6#:检查new 操作的结果是否为null Tk.MtIs)V}
Q}\,7l
7 &GhJ^Ku
_f^q!tP&d
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: =Q3Go8b4HJ
<mrLld#_:C
9DKmXL
$AG.<
Integer i = new Integer (400); gq Z7Pro.
if (i == null) uZd)o
AB
throw new NullPointerException(); ;)"r^M)):
MSRIG-
CEqfsKrsxE
wpx,~`&
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 )z7.S"U
P63z8^y
if#$wm%
EU>@k{Qt
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 -_>c P
w>/KQ> \"
七、常见错误7#:用== 替代.equals >[ lj8n
d 'x;]#S
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: 8V=I[UF.1?
8Q#&=]W$
<pK;D
V&h,v%$
int x = 4; eA{,=,v)
int y = 5; t
m5>J)C
if (x == y) 9L!Vj J
System.out.println ("Hi"); 4.H!rkMM
// This ’if’ test won’t compile. ``aoLQc`
if (x.equals (y)) 47$JN}qI0
System.out.println ("Hi"); >s[}f6*2@
c{||l+B
mc!3FJ
YwB5Zqr
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 GN=F-*2
~;bwfp_
w<\N-J|m
O}IS{/^7
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 bsqoR8
D=9x/ ) *G
>6jyd{
.!)7x3|$[
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 BN#^
/a-
mI0|lp 1$
ks(PH6:]<
pSV
8!
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 z81I2?v[Jr
Jv7 @[<$
P3lNns3
4fP>;9[F
八、常见错误8#: 混淆原子操作和非原子操作 r10)1`[
mN@0lfk;
:*}tkr4&eh
~a/yLI"'g
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: !B-&I E?
`DWzp5Ax
P d*}0a~
B<:i[~`7t
public class Example{ b!7"drge:
private int value; // More code here... CZwZ#WV6
public void set (int x){ #i)h0ML/e
// NOTE: No synchronized keyword H(?z?2b p
this.value = x; u@==Ut
} 'e{e>>03
} VMen:
+k8><_vr}
9;h1;9sC|
EWH'x$z_q
不过,这个保证仅限于读和写,下面的代码不是线程安全的: 7J$ ^R6rh
3@6f%Dyj
@jwUH8g1
6
D!,vu
public void increment (){ ;]<$p[m
// This is effectively two or three instructions: mRQ F5W6
// 1) Read current setting of ’value’. .0\Wu+
// 2) Increment that setting. y6:=2(]w<p
// 3) Write the new setting back. `@Kh>K
++this.value; {/#?n["
} atl0#F Bd
{wA@5+[
BT`/OD@
<
> f12pu
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: hr]NW>;
1iF
|t5>e
WGp81DNS|
0m*0I>
public synchronized void increment (){ *pI3"_
++this.value; 2"V?+Hhz
} Zu~ #d)l3N
puMpUY
';b/D
<7^_M*F9
九、常见错误9#:在catch 块中作清除工作 (sr_&7A
/l:3*u
PPE:@!u<
,JVD ;u
一段在catch块中作清除工作的代码如下所示: L$(W*
PG}
mjy%xzVr6^
3R4-MK
d@] 0 =Ax
OutputStream os = null; PX]A1Kt?
try{ z
KJ6j ]m
os = new OutputStream (); &a48DCZ
// Do something with os here. }>)"!p;t_
os.close(); wPqIy}-
}catch (Exception e){ Qj0@^LA
if (os != null) ZH&%D*a&
os.close(); |? r,W~9`
} c#CX~
;[dcbyu@
>@TZYdl
!>t|vgW
尽管这段代码在几个方面都是有问题的,但是在测试中很容易漏掉这个错误。下面列出了这段代码所存在的三个问题: rJ!xzge;G
p|AIz3
K6|*-Wo.
'lIT7MK
1.语句os.close()在两处出现,多此一举,而且会带来维护方面的麻烦。 :/Sx\Nz78
)(75dUl
7b'XQ/rs
`n5|4yaG~
2.上面的代码仅仅处理了Exception,而没有涉及到Error。但是当try块运行出现了Error,流也应该被关闭。 "p$`CUtI
]
J:^$]
hnG'L*HooE
Z;??j+`Eo
3.close()可能会抛出异常。 :LcR<>LZ
w_\niqm<y
K*CO%:,-
cB?HMLbG>
上面代码的一个更优版本为: >cSc
q]
,&$d^@
3G5i+9Nt.L
Ij{{Z;o3
OutputStream os = null; WERK JA
try{ rxm!'.+
os = new OutputStream (); vco:6Ab$
// Do something with os here. )v
['p
}finally{ uCUQxFp
if (os != null) ?~u"w OH'
os.close(); {!6!z,
} qZA?M=NT?
Ibpk\a?A{
G9}[g)R*
/r}t
这个版本消除了上面所提到的两个问题:代码不再重复,Error也可以被正确处理了。但是没有好的方法来处理第三个问题,也许最好的方法是把close()语句单独放在一个try/catch块中。 E!3W_:Bs
-
n11L
n%Nf\z
a.c2ScXG
十、常见错误10#: 增加不必要的catch 块 ]6$NU
[
r=qb[4HiV
e&]XiV'
"t4~xs`~X
一些开发者听到try/catch块这个名字后,就会想当然的以为所有的try块必须要有与之匹配的catch块。 xNq&_oY7
F/@#yQv?
N:gS]OI*
JUwP<C[
C++程序员尤其是会这样想,因为在C++中不存在finally块的概念,而且try块存在的唯一理由只不过是为了与catch块相配对。 (lEWnf=2h
0W]Wu[k
d [K56wbpx
9[$g;}w
增加不必要的catch块的代码就象下面的样子,捕获到的异常又立即被抛出: Kw925@W
f9OVylm
VbA#D 4;
9{ciD
"!&V
try{ (AR-8
// Nifty code here ,'82;oP4
}catch(Exception e){ Zf(ucAhL
throw e; 8]2S'mxE
}finally{ #M{}Grg
// Cleanup code here 0g`WRe
} n6ud;jN|
O6boTB_2
G7zfyw}W
C"hc.A&4
不必要的catch块被删除后,上面的代码就缩短为: gKS^-X{x
tTQ>pg1{qh
T[ky7\
/mqEc9sq,
try{ SU
H^ ]4>
// Nifty code here uOm fpg O
}finally{ r1F5&?{q
// Cleanup code here J+Y&