代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 @B ?'Mu*
_U,Hi?b"$}
_BCq9/
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 V,?])=Ax
'c
>^Aai
-afNiNiY
Z]Cd> u
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 tCu.Fc@
azRp4~2?
<^ratz!-
5UG"i_TC
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 SR<W3a\
f/m0,EERk
%"|W
qxv
j|[ >f
一、常见错误1# :多次拷贝字符串 0^F!-b^z
Ypinbej
WP^wNi
~>
3XncEdy_
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 \#C]|\
3aJYl3:0B
^5k~7F.
f'Oj01[
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: QQ %W3D@
Nkn2\w
i
nk!>Z
o Z%oP V:
String s = new String ("Text here"); u!t<2`:h
g?@fHFct
R\x3'([A5
QM7BFS;
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: $Xs`'>,"
B!4~A{
Sb }=j;F
&&zsUAkS
String temp = "Text here"; :xY9eq=
String s = new String (temp); eEYzA
#W4
" ^#2
z]&?}o
cXb&Rm'L
但是这段代码包含额外的String,并非完全必要。更好的代码为: ~*e@^Nv)v
%Vk77(
u7},+E)+B
!bCaDTz
String s = "Text here"; vb9C
fGWXUJ
_s}`ohKvD
8/lgM'Eux
二、常见错误2#: 没有克隆(clone)返回的对象 zu
7Fq]zD
D J7U6{KLq
VL|Z+3L
((=T E
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: v^Rw9*w{
!1Ht{cA0
Q^X}7Z|T
7"OJ,Mx%
import java.awt.Dimension; Zh`[A9I/
/***Example class.The x and y values should never*be negative.*/ ?.Ip(g
public class Example{ 1Vs>G
private Dimension d = new Dimension (0, 0); fm!\**Q1
public Example (){ } bRr3:"=sE
`A-
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ M
C y~~DL
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ MSB/O.
if (height < 0 || width < 0) }i^$
li@
throw new IllegalArgumentException(); ]<S{3F=
d.height = height; hS&.-5v
d.width = width; 3PlIn0+LX
} [GcW*v
.Ad9(s
public synchronized Dimension getValues(){ lbC,*U^
// Ooops! Breaks encapsulation Z*=$n_
G
return d; AJ)&+H
} g{f7} gTG
} LjIkZ'HuF
B>@l(e)b
5Vai0Qfcu:
wJ"]H!r0
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码:
;v/un
UD9JE S,
dN7.W
0
ZSn r+
Example ex = new Example(); CTxP3a9]
Dimension d = ex.getValues(); \[]?9Z=n
d.height = -5; }Yc5U,A;
d.width = -10; S)/548=`
:6/$/`I0W
z;_vl
q([{WZ:6Oq
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 &9"Y:),
1U?5/Ja
LF#[$
so{i
!~V^GlY
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 %F^,6y
?'tRu !~
be$']}cP
1N<n)>X4
更好的方式是让getValues()返回拷贝: CxSh.$l
5:C>:pA V
Et0)6^-v
[HL>Lp&A?
public synchronized Dimension getValues(){ ) .KMZ]
return new Dimension (d.x, d.y); 6 N:Ps8Hg
} PB!XApTb
e
m0 hTxb
{m@tt{%
/Z,hQ>/
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 dCo3 VF"u
B|,d
pf%;*
"_&c[VptWi
三、常见错误3#:不必要的克隆 yqVoedN
g-1j#V`5
)[|_q,
cbs ;
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: yz5! >|EB
F^aD#
5G$ ,2i(
=\oL'>q
/*** Example class.The value should never * be negative.*/ E!BzE_|i
public class Example{ L]u^$=rI
private Integer i = new Integer (0); 1iNMgA
public Example (){ } x+;y0`oL
ST0TWE'
/*** Set x. x must be nonnegative* or an exception will be thrown*/ jA[Ir3
public synchronized void setValues (int x) throws IllegalArgumentException{ M)nh~gU
if (x < 0) xplV6q`
throw new IllegalArgumentException(); u6Wan*I?
i = new Integer (x); MLg{Y?@
} K+XUC
;R
Jv7@
public synchronized Integer getValue(){ >tx[UF@P@
// We can’t clone Integers so we makea copy this way. D.|r
[c
return new Integer (i.intValue()); qvK/}
} dv+ZxP%g
} 8 H3u"
c b&Yf1
0:<Y@#L
{@3v$W~7M
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 <VutwtA
+6
=lN[b
BV
}CmU&DA
CV_M |
方法getValue()应该被写为: RqP_^tB
`wQs$!a
BzkfB:wr
i3Bpim.
public synchronized Integer getValue(){ -mn/Yv
// ’i’ is immutable, so it is safe to return it instead of a copy. :#35mBe}k
return i; 1q3"qYH
} =QbOvIq
XWQ `]m)
lX)AbK]nb
EP>Lh7E9n
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: _cj=}!I
t[|t0y8
U]_WX(4 @
6M_:D
?Boolean QKB+mjMH#x
?Byte ]'M B3@T
?Character L jTSu9I>
?Class 8ih_S2Cd
?Double qNP)oU92
?Float ;UM(y@
?Integer WZPj?ou`G
?Long Wf"GA i
?Short wytMoG\
?String 1[u{y{9 q
?大部分的Exception的子类 7aKI=;60.
VjNr<~ |d
la w$LL
|N"K83_pr
四、常见错误4# :自编代码来拷贝数组 SA&(%f1d
( $2M"n
DB-79U %W
?[m1?
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: k'H[aYMA
KV|D]}
*AQ3RA 8
zow8 Q6f
public class Example{ ;-@: }/
private int[] copy; ")\V
/*** Save a copy of ’data’. ’data’ cannot be null.*/ LjE3|+pJ
public void saveCopy (int[] data){ C$~ly=@
copy = new int[data.length]; ,>7dIJqzw
for (int i = 0; i < copy.length; ++i) !#W>x49}
copy = data; AG9DJ{T
} `iM%R3&
} 9N)I\lcY
SO(BkxV@
saP%T~
z,x
)Xx
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: y*{zX=]l<
:i?6#_2IC
Q2r[^Z
5$'[R;r
void saveCopy (int[] data){ r(uo-/7z
try{ R2a99# J
copy = (int[])data.clone(); K-#d1+P+
}catch (CloneNotSupportedException e){ wn! =G~nB
// Can’t get here. J8r8#Zz
} O!f37n-TB
} }*QK;#NEc
RH<2f5-sC!
hx9t{Zi
?{aJ#w
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: yTg|L9
7:pc%Ksq
u%z'.#r; a
y-nv#Ejr
static int[] cloneArray (int[] data){ ;#9?3Os
try{ gFHBIN;u
return(int[])data.clone(); U!r8}@
}catch(CloneNotSupportedException e){ 1_q!E~)
// Can’t get here. B!x#|vGXL
} %E&oe $[B
} `MPR-"Z6
\L~^c1s3r
s.Z{mnD6
r[}nr H&8
这样的话,我们的saveCopy看起来就更简洁了: nng|m
\}=T4w-e
dLb$3!3
|rk.t g9
void saveCopy (int[] data){ T\.(e*hC
copy = cloneArray ( data); RVwS<g)~1
}
2hF^U+I}
sU&v
B:]~
1 mJUlx
m=@xZw<
五、常见错误5#:拷贝错误的数据 c:0n/DC
n5UUoBv
vkhPE(f
<#?dPDMG.*
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: ^SG>VfgC
!`?i>k?Q E
\&kj#)JYA
F}45.CrD
import java.awt.Dimension; yXDjM2oR/2
/*** Example class. The height and width values should never * be NN'pBUR
negative. */ `9[n5-t
public class Example{ >YWK"~|i~
static final public int TOTAL_VALUES = 10; BT8)t.+pv
private Dimension[] d = new Dimension[TOTAL_VALUES]; _`;KmD&5
public Example (){ } m0ra
OeASB}
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ mm+V*L{x
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ K\%\p$ZD
if (height < 0 || width < 0) & tT6.@kH
throw new IllegalArgumentException(); :
pUu_
if (d[index] == null) zCo$YP#5_
d[index] = new Dimension(); vFdI?(c-
d[index].height = height; B7'#8heDh
d[index].width = width; vb>F)po1}
} DVhBZ!u9
public synchronized Dimension[] getValues() p1d%&e
throws CloneNotSupportedException{ Pv2uZH(
return (Dimension[])d.clone(); myX&Z F_9
} Wfd`v
} [XI:Yf
2qE_SSXn
:$K=LV#Iru
!J;Bm,Xn6
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: ,i}EGW,9q
TPBQfp%HU
\\:%++}J
QObVJg,GD
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ vv,<#4d
Dimension[] copy = (Dimension[])d.clone(); a_}C*+D
for (int i = 0; i < copy.length; ++i){ e< @$(w
// NOTE: Dimension isn’t cloneable. &PV%=/-J
if (d != null) ${z#{c1
copy = new Dimension (d.height, d.width); %$zak@3%'
} `=hCS0F
return copy; \jk*Nm8;
} $Q#n'#c
t^MTR6y+8
vd#)+
bi}aVtG~z
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: HRE?uBkjf
7P3/Ky@6
Ewkx4,`Ff
3AdYZ7J
public void store (int[] data) throws CloneNotSupportedException{ ",aNYJR>*!
this.data = (int[])data.clone(); ^wZx=kas
// OK jRiMWolLv
} e)?}2
g#74c'+
p+?`ru
>F7HKwg}Z
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: ,rN$ah$CL
U5j4iz'
zMp vS rc
N}nE9z5
public void wrongStore (int[][] data) throws CloneNotSupportedException{ u*%mUh
this.data = (int[][])data.clone(); // Not OK! "#pxZ
B=
} u0+F2+ I
public void rightStore (int[][] data){ [[T6X9
// OK! egA*x*8
this.data = (int[][])data.clone(); Xa>'DO2
for (int i = 0; i < data.length; ++i){ mgH~GKf^
if (data != null) ;&|I/MVm
this.data = (int[])data.clone(); #9}1Lo>
} +@fEw
} 9C?SEbC
:!t4.ko
:Wx7a1.Jz
]Zh$9YK
*%jtcno=Y
六、常见错误6#:检查new 操作的结果是否为null ehB'@_y
7&P70DO
M]Vi]s
Ppl :_Of
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: p9G+la~;VM
V[%IU'{:
o`QH8
)d3C1Pd>
Integer i = new Integer (400); z"|jCdZGM
if (i == null) @^ta)Ev
throw new NullPointerException(); }sqFvab<
X4\T=Q?uLx
X6B,Mply
E|-5=!]fX
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 "h1ek*(?<
~~&Bp_9QXN
k9H}nP$F
X)j%v\#`U
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 I8@leT\9M
C%<Dq0j
七、常见错误7#:用== 替代.equals wpN [0^M-0
P}Mu|AEG
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: tkptm%I_
;m|N9'
"* FjEA6=
b(U5n"cdA
int x = 4; R86i2',
int y = 5; ]==7P;_-
if (x == y) yRQ1Szbjli
System.out.println ("Hi"); v;z8g^L
// This ’if’ test won’t compile. :rufnmsP<U
if (x.equals (y)) !W&|kvT^
System.out.println ("Hi"); )xg8#M=K
@S~n^v,)
hB>FJZQ_
8N j}
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 @HI@PZ>
]o'dr
r
VFq\{@-
%
7,?ai6{
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 &