代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 SWtqp(h]'
GsO(\hR6^
"kFNOyj3\
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 }x#e.}hf&
JS03BItt
XlX t,
Pc?"H!Hkn
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 t!xdKX& }
leF!Uog
g3Q;]8Y&
y<HNAGj
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 o;DK]o>kH
W2%@}IDm
+mft
q`8
5-
一、常见错误1# :多次拷贝字符串 HP7~Zn)c
0`V=x+*,
0i5S=L`j
@8w[Z o~
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 EhKG"Lb+
#Mk3cp^Yl
xVYa-I[Z
Z0M,YSn z
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: iW2\;}y
fVZ92Xw
B
^?0'\Z
W8x&:5Fc)3
String s = new String ("Text here"); wQ/.3V[
z&c}
Qe!3ae`Z
}Z\S__\9
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: *qYw
)n<p_vz
o&M.9V?~~
_PGd\>Ve
String temp = "Text here"; W!"QtEJ,
String s = new String (temp); !5h8sD;
d"E3ypPK
+BO kHXk1
-awG14%
但是这段代码包含额外的String,并非完全必要。更好的代码为: Kwm_Y5`A
X.
Ur`X
LN.*gGl
EUh_`R
String s = "Text here"; x|AND]^Q
.nNZdta&=
MSBrI3MqQ
mJ(ElDG
二、常见错误2#: 没有克隆(clone)返回的对象 7;Lv_Y"b
Xf"<
>M
O8>&J-+2
raSga'uT;
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: +84
p/B#
p(="73
AEx VKy
0Ntvd7"`}
import java.awt.Dimension; eU`O=uE
/***Example class.The x and y values should never*be negative.*/ ^7i7yM}6(
public class Example{ h{zb)'R
private Dimension d = new Dimension (0, 0); =_j<x$,b-
public Example (){ } Al@. KTK
Q`.q,T8I
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ r|]YS6
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ WrRY3X
if (height < 0 || width < 0) BHU$QX
throw new IllegalArgumentException(); {jwLVKT$
d.height = height; x)N QRd
d.width = width; VR1[-OE
} z6;hFcO
&w`DF,k|
public synchronized Dimension getValues(){ Q {~$7J
// Ooops! Breaks encapsulation $B<:SuV#
return d; m]}U!XT
} =vQ J2Rg
} lIx./Nf
?WqaT) l~
:x5O1Zn/t
]9_}S
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: IC8%E3
,~1sZ`C
01&E.A
5}w
Example ex = new Example(); -I6t ^$HA
Dimension d = ex.getValues(); Og@{6>
d.height = -5; JKrS;J^97v
d.width = -10; XjP;O,x
imzPVGCD{
u)r:0;5
@TXLg2
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 Ac*J;fI
\/\w|j
.m\0<8C
r_,;[+!
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 ;PMh>ZE`
D *PEIsV
3iX\):4
`$6~QLUf
更好的方式是让getValues()返回拷贝: H[OgnnM
IoK/ 2Gp
}a9G,@:k
"lt5gu! `u
public synchronized Dimension getValues(){ :/Es%z
D
return new Dimension (d.x, d.y); %yjD<2J;
} v[8+fd)}S
T2.[iD!A
q{B?j%.o
n|rKo<Y0
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 ~LOE^6C+~o
IFS_DW
q3h&V
dT?3Q;>B?
三、常见错误3#:不必要的克隆 T,>L
nfGI4ZE
lQ!OD&6
%.$7-+:7A
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: t&[<Dl/L
>nih:5J,ja
H=f|X<8
]b sabS?
/*** Example class.The value should never * be negative.*/ mK"s*tD
public class Example{ to,\n"$~!
private Integer i = new Integer (0); 5E~^-wX
public Example (){ } Xxd]j]
@@{5]Y
/*** Set x. x must be nonnegative* or an exception will be thrown*/ >zL5*:G
public synchronized void setValues (int x) throws IllegalArgumentException{ m_Q&zp["
if (x < 0) _!,
J iOI
throw new IllegalArgumentException(); q-_!&kDK"
i = new Integer (x); ^->S7[N?
} nu-&vX
:E~rve'
public synchronized Integer getValue(){ \M._x"
// We can’t clone Integers so we makea copy this way. BL_0@<1X
return new Integer (i.intValue()); /T(9:1/G
} > l0H)W
} 2u!&Te(!9
rJCb8x+5a
gM=:80
!3mt<i]a"
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。
#C?M-
sO6=w%l^
iQ2j ejd3(
S
>CKm:7
方法getValue()应该被写为: 6},[HpXRc4
|m
?ZE:
^w.]1x
P#XV_2
public synchronized Integer getValue(){ 0('ec60u
// ’i’ is immutable, so it is safe to return it instead of a copy. ,J!$Q0 e
return i; !8cV."~
} >-<iY4|[d
V(DY!f_%
j4!O,.!T
;`:YZ+2
Z
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: ArNQ}F/
"2sk1
0NC70+4L
fbOqxF"?we
?Boolean )=29Hm"
?Byte 2@GizT*mA
?Character ^rP]B-)
?Class Km%L1Cd]
?Double .#|?-5q/iN
?Float Q!U}
?Integer PS[ C!s&KE
?Long urjf3h[%
?Short 8j3Y&m4^
?String X|eZpIA45
?大部分的Exception的子类 )S2yU<6oOt
_(kaa WJ
0.n[_?<(
flFdoEV.U)
四、常见错误4# :自编代码来拷贝数组 1nskf*Z
\P?X`]NwnO
tZKw(<am
$Emu*'
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: N~mr@rXC
uij^tN%
RLnL9)`W
Im/tU6ybV
public class Example{ '= fk;AiQ
private int[] copy; %60 OS3
/*** Save a copy of ’data’. ’data’ cannot be null.*/ I_u/
public void saveCopy (int[] data){ N6}/TbfAR
copy = new int[data.length]; BrwC9:
for (int i = 0; i < copy.length; ++i) k_0@,b3
copy = data; HRDpFMA/~
} p.=9[`
} ;t\h"K<,|
}A24;'}
&gY) x{
CYW@Km{e
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: $%cc[[/U
9 =;mY
4#0 3x:/<\
=ZIT!B?4
void saveCopy (int[] data){ 6,3o_"J!
try{ crP2jF!
copy = (int[])data.clone(); d"#Zp
}catch (CloneNotSupportedException e){ !ou#g5Q@z
// Can’t get here. ~,HFd`
} qEST[S V
} J}X{8Ds9
ls]N&!/hq
V<0iYi;4=
H,X|-B
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: +ZOiL[rS
ob{'Z]-V
'|^:,@8P9
!`Rh2g*o9
static int[] cloneArray (int[] data){ /;Tc]
try{ UPfO;Z`hJ
return(int[])data.clone(); s.}K?)mH
}catch(CloneNotSupportedException e){ \7/yWd{N$
// Can’t get here. -Qn7+?P
} ]19VEH
} *n? 1C"l
{G:y?q'z
w!7\wI[
!rM~
这样的话,我们的saveCopy看起来就更简洁了: 1jl!VU6
EbQLMLD%
.9qK88fU R
lZ\8W^
void saveCopy (int[] data){ uPA
(1
copy = cloneArray ( data);
|/*Pimk
} F`nQS&y
;[sW\Ou
S }`sp[6
J/?Nf2L4
五、常见错误5#:拷贝错误的数据 pFd8p@m_2
"n!yK
)S)L9('IxT
37/n"\4
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: `@h|+`h
yJm"vN
aKbmj
]yU"J:/
import java.awt.Dimension; vjGQ! xF
/*** Example class. The height and width values should never * be 0Z9DewwP
negative. */ d!y*z
public class Example{ Dsg>~J'
static final public int TOTAL_VALUES = 10; \c}(rqT
private Dimension[] d = new Dimension[TOTAL_VALUES]; dw
bR,K
public Example (){ } 5~JT*Ny
H$(bSw$
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ ;<AcW.jx
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ EiW|+@1
if (height < 0 || width < 0) do}LaUz
throw new IllegalArgumentException(); jmM|on!
if (d[index] == null) i<m(neX[H
d[index] = new Dimension();
Pd*[i7zhC
d[index].height = height; I0)`tQ+
d[index].width = width; rVYoxXv
} >1~
/:DJ
public synchronized Dimension[] getValues() <$(B [T
throws CloneNotSupportedException{ ^/2I)y]W0
return (Dimension[])d.clone(); k"(]V
} 0M_oFx
} ?['!0PF
}vd*eexA
4>hHUz[_
iq(PC3e`V
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: 'pdTV:]zA
@X2*O9
|p11Jt[
:S+K\
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ [. 5m}V
Dimension[] copy = (Dimension[])d.clone(); 6[> lzEZ
for (int i = 0; i < copy.length; ++i){ !~@GIr
// NOTE: Dimension isn’t cloneable. UNdD2Fd9
if (d != null) A =Wg0eYy\
copy = new Dimension (d.height, d.width); m~ tvuz I
} E7fx4kV
return copy; T~TP
} (yOkf-e2y
1o_kY"D<
BM%wZ:
s
h+ f>#O+:
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: E9HMhUe
> VG
H",B[
YK
_'u]{X\k{J
public void store (int[] data) throws CloneNotSupportedException{ a|aVc'j
this.data = (int[])data.clone(); bLgH3[{
// OK /:&!o2&1H
} Lsmcj{1d
^PksXfk
J3K=z
RgE`H r
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: "/#JC}]
tT$OnZu&
l\HdB"nT
`BPTcL<W
public void wrongStore (int[][] data) throws CloneNotSupportedException{ 4`IM[DIG~
this.data = (int[][])data.clone(); // Not OK! y7R#PkQ~
} ou|emAV
public void rightStore (int[][] data){ DX>a0-Xj
// OK! W?
iA P
this.data = (int[][])data.clone(); Qw5nfg3T
for (int i = 0; i < data.length; ++i){ Wgq|Q*
if (data != null) XH:*J+$O
this.data = (int[])data.clone(); z*y!Ml1
} `&$8/_`
} GXNf@&
[|u^:&az
8sG3<$Z^
$Gn.G_"v
n\#YGL<n
六、常见错误6#:检查new 操作的结果是否为null N+-Tp&:wY
XZ
rI w
1SQATUV
!*IMWm>
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: ~}/Dl#9R!
G7-BeA8
R`?l.0
4JSPD#%f
Integer i = new Integer (400); mYBEjZB
if (i == null) /'O8RUjN
throw new NullPointerException(); g;IlS*Ld
T)C@6/
}kbSbRH43
-+9[X*VCc
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 adON&<
bQll;U^A
B*7kX&Uq
cw;wv+|k
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 ZO}Og&%
$|4C]Me (
七、常见错误7#:用== 替代.equals l?Y^3x}j
`sxfj)s
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: D)_
C@*q
Rd?}<L
k_=SDm a
NzRvb j]
int x = 4; jXcJ/g(X3
int y = 5; OI R5QH
if (x == y) QaX.Av
System.out.println ("Hi");
w-jElV
// This ’if’ test won’t compile. 0MQ= Rt
if (x.equals (y)) z(PUoV:?
System.out.println ("Hi"); l/$GF|`U
_Fb}zPU!
0QQss
~m%[d.
}e
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 vL/ 3(Bo7
^}3^|jF
~PQ.l\C
fJ,N.O+9E
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 ]5W$EvZ9)
WxdQ^#AE
61\u{@o$
!Jg;%%E3:i
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 ,RP-)j"Wff
aCQtE,.
\lL[08G
z,HhSW?&^
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 Jx*cq;`Vee
V>$( N/1
^df wWP
0V%c%]PH
八、常见错误8#: 混淆原子操作和非原子操作 iT@`dEZ.
5s7BUT
CB7dr&>
=j]y?;7q
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: w+o5iPLX
];r!
M0
{f*Y}/@
\BOoY# !a
public class Example{ ,|%KlHo^
private int value; // More code here... :\](m64z;
public void set (int x){ *miG<
// NOTE: No synchronized keyword #ydold{F
this.value = x; GR,2^]<{
} [hJ1]RW8
} 6fwNlC/9
01bCP
$Dg-;I
l![M,8
不过,这个保证仅限于读和写,下面的代码不是线程安全的: ~NGM6+9
rOIb9:
i4C{3J^
?2<QoS
public void increment (){ ",r
v%i2 f
// This is effectively two or three instructions: G
hM
// 1) Read current setting of ’value’. #h!+b
// 2) Increment that setting. c
'|*{%<e2
// 3) Write the new setting back. |jsI-?%8J
++this.value; ktu?-?#0,
} RK# 6JfC3X
!E70e$Th
B`pBIUu
;W|kc</R*
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: N,t9X7G&
KbJ6U75|f
o<g?*"TRh
/%$Zm^8c
public synchronized void increment (){ LUbhTc
++this.value; +cpb!YEAb
} 1nVQYqT_
2g(_Kdj*{
qLR;:$]Q&8
+in)(a.
九、常见错误9#:在catch 块中作清除工作 ?pL|eS7
tX*@r
B=Hd:P|
]&'!0'3`
一段在catch块中作清除工作的代码如下所示: o.s'0xP]
(6,:X
AvL /gt:
)1 <0c@g=
OutputStream os = null; PW*Vfjf4
try{ x;ik
os = new OutputStream (); {uDW<