代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 m]V5}-?al
&a O3N
Zl]\sJ1"
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 cU+/I>V
#Ez>]`]TB
($]y*|Obn
9NVe>\s_
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 fAJQ8nb{@]
,1od]]>(O
1Ocyrn
ZNzye1JSm
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 @ %kCe>r
IGVNX2
%U'YOE6
b{9q
一、常见错误1# :多次拷贝字符串 m39 `f,M
W0X?"Ms|a
5`0tG;
;A1pqHr
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 Ig]Gg/1G
\9!W^i[+
;g*ab
p1CY?K
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: ?DA,]aa-
ZY=x$($f
UT+B*?,h
z>hA1*Ti
String s = new String ("Text here");
|G{TA
7\eN8+
{p+7QlgK
Lylw('zZ
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: C;M.dd
*s<FE F
!|hv49!H
2?#IwT'
String temp = "Text here"; na_Y<R`
String s = new String (temp); }h>QkV,{2
]k5l]JB
8I3"68c_a
<S%M*j
但是这段代码包含额外的String,并非完全必要。更好的代码为: -Y{P"!p0
nUD)G<v
ZEp UHdin
IA!( 'Ks
String s = "Text here"; 7i,}F|#8
* 3WK`9q
YeK PoW
1W;q(#q
二、常见错误2#: 没有克隆(clone)返回的对象 `A])4q$
j!xt&t4D
b&. o9PV"
/X{:~*.z
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: =EgiV<6vcH
C|8.$s<
J[du>1D
s9?klJg
import java.awt.Dimension; H"6Sj-<=
/***Example class.The x and y values should never*be negative.*/ w-pdpbHV
public class Example{ ]G#og)z4
private Dimension d = new Dimension (0, 0); t?iCq1
public Example (){ } +v{<<
@;!s"!~sv
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ "JT R5;`w
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ g`k_o<'JC
if (height < 0 || width < 0) 43^%f-J5
throw new IllegalArgumentException(); eJIBkFW/3y
d.height = height; +h.$<=
d.width = width; |]w0ytL>(2
} {=VauF
$_%yr
~2
public synchronized Dimension getValues(){ MS)(\&N
// Ooops! Breaks encapsulation *2Il{KOA^
return d; |MY6vRJ(
} Ol"*(ea-TX
} 615, P/
bzz=8n
<H::{
!7]4sXL{
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: 80U07tJ
]W-l1
P33x/#VVE
nJ<h}*[
Example ex = new Example(); >r6`bh
[4
Dimension d = ex.getValues(); S;[9
hI+
d.height = -5; (hEqh
nnm`
d.width = -10; g-q~0
#p_3j 0S
4{7O}f
s~W:N.}*
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 CA, &R<]
+}@1X&v:
b`)^Ao:
#8H
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 rZm|7A)i
VC,wQb1J/
HdQd =q(
pHSq,XP-
更好的方式是让getValues()返回拷贝: !)FM/Xj,o
8pp^
w
4RTuy+
M
W7r1!/ccj
public synchronized Dimension getValues(){ dt%waM!
return new Dimension (d.x, d.y); C%}}~Y
} gh>'O/9
<1cYz\/!M
*J&XM[t
ZbD_AP
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 rP Wn
ppH5>Y
6c
?~s,O$o
\x\_I1|
三、常见错误3#:不必要的克隆
*(5y;1KU
!B_i~Rmg
~Q}JC3f>
rw/WD(
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: ]c%yib
})f4`$qf
B/u0^!
JFf*v6:,
/*** Example class.The value should never * be negative.*/ r*CI6yP
public class Example{ .cJWYMC
private Integer i = new Integer (0); MdM^!sk&`
public Example (){ } )D?\ru H
/V}>v
/*** Set x. x must be nonnegative* or an exception will be thrown*/ *Y(v!x \L
public synchronized void setValues (int x) throws IllegalArgumentException{ |>(d^<nR^v
if (x < 0) X~wkqI#d%E
throw new IllegalArgumentException(); JsAl;w
i = new Integer (x); hqOy*!8'@
} w],+l N;
Y?G\@6
public synchronized Integer getValue(){ 6B>1"h%Wf
// We can’t clone Integers so we makea copy this way. RQiGKz5
return new Integer (i.intValue()); ,w&8 &wj
} zG)XB*c
} j}}:&>;
|eH>55 b
Ct2m l
IO3`/R-
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 ?\[2Po]n
#'m&<g,
} m5AO 4:
?`T<
sk8c
方法getValue()应该被写为: :KY920/,
)*<=:
V8AF;1c?-'
CZaUrr
public synchronized Integer getValue(){ (s`oJLW>
// ’i’ is immutable, so it is safe to return it instead of a copy. tg%s#lLeH
return i; i
Lr*W#E
} pd2Lc
$O@
-XNjyXm2
.PjJ g^^
78a!@T1#
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: ?z,^QjQ}
.<uxZ
4wfT8CL
{@j0?s
?Boolean +|6
'7Z(9
?Byte &%3$zgvR
?Character 2ZHeOKJ-
?Class t[gz#'
?Double Gd^K,3:. T
?Float 29eg.E
?Integer Z(g9rz']0
?Long FnkB
z5D
?Short Z#H] yG
?String q:2V w`g'
?大部分的Exception的子类 9v[cy` \
x\HHu]
t\YN\`XD
d:KUJ
Y.
四、常见错误4# :自编代码来拷贝数组 Y4E UW%
Tc{r;:'G<
UG)J4ZX
nT
UKA
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: X;$g7A
^@W98_bd;
*5KV DOd
} Ej^M~Vv
public class Example{ 00s&<EM
private int[] copy; )na8a!
/*** Save a copy of ’data’. ’data’ cannot be null.*/ 7PE3>cD
public void saveCopy (int[] data){ Vq[L4
copy = new int[data.length]; GJlkEWs
for (int i = 0; i < copy.length; ++i) r8PXdNg
copy = data; ;uw`6 KJ
} wk
@-O}W
} eK]g FXk
M#v#3:&5
8S;]]*cD~
|a!AgvNF
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: P_:A%T
dOm`p W ^
Z.9?u;
+RIG8w]
void saveCopy (int[] data){ ziFg+i%s
try{ B^4D`0G[4
copy = (int[])data.clone(); j9)WInYc:
}catch (CloneNotSupportedException e){ 3@u<Sa
// Can’t get here. a%3V<
"f
} L`"PaIMz
} G01 J1Ll}
XL@Y!
5HWVK .
CH
|A^!Zm
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: OGmOk>_
:4o08M%
zk)9tm;i{
Q_p!;3
static int[] cloneArray (int[] data){ \SB~rz"A
try{ p7.j>w1F
return(int[])data.clone(); -w8c;5X
}catch(CloneNotSupportedException e){ 8Lm}x_
// Can’t get here. K@ZK@++
} :]?y,e%xu,
} r >%reS
Dx<">4
gQ]WNJ~>
P( z#Wk
这样的话,我们的saveCopy看起来就更简洁了: 8;'fWV?
U
{+Rf?'JZH
YS$?Wz
R-xWZRl>
void saveCopy (int[] data){ QBi&Q%p iy
copy = cloneArray ( data); k_A. aYe
} 1UR;}
[3Qu @;"&
mDn*v(
f
R-v99e iN
五、常见错误5#:拷贝错误的数据 l}|KkW\y
JryC L]
eURy]
]k2Jf}|
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: jI`1>>N&1
aBV{Xr~#(
%m\dNUz4g
,^dyS]!d$
import java.awt.Dimension; SoS GQ&k
/*** Example class. The height and width values should never * be vo'=d"zm
negative. */ yn;h.m [):
public class Example{ V?{[IMRC
static final public int TOTAL_VALUES = 10; -49z.(@ki
private Dimension[] d = new Dimension[TOTAL_VALUES]; d1=kHU4_9
public Example (){ } !1MSuvWP
]?<j]u0J
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ O0?.$f9 s
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ p h[
^ve
if (height < 0 || width < 0) z"`q-R }m
throw new IllegalArgumentException(); 3`9H
if (d[index] == null) D;@*
d[index] = new Dimension(); eQBR*@x
d[index].height = height; I+ZK \?Rs
d[index].width = width; =ytB\e
} '\[o>n2
public synchronized Dimension[] getValues() kNX"Vo]1
throws CloneNotSupportedException{ ^X$k<n A;
return (Dimension[])d.clone(); igNZe."V
} 2i+'?.P
} &<</[h/B/F
n-q
'qRK6}"T
E\U6n ""]
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: @^Tof5?F?
l#8SlRji
0Xmp)_vba
!2dA8b
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ a}N m;5K
Dimension[] copy = (Dimension[])d.clone(); u!in>]^
for (int i = 0; i < copy.length; ++i){ 79:Wo>C3-
// NOTE: Dimension isn’t cloneable. mmC&xZ5f
if (d != null) YmP`Gg#>p
copy = new Dimension (d.height, d.width); 3JuWG\r)l
} dQfVdqg
return copy; i#I+
} B7"/K]dR:
?`+46U%
P.bBu
cnm&oC 6
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: b-}nv`9C
>h3r\r\n3
+dWx?$n
\I
r&&%
public void store (int[] data) throws CloneNotSupportedException{ Y,O)"6ev
this.data = (int[])data.clone(); R:+2}kS5e{
// OK r&AX
} 74*1|S<
}]w/`TF
e|:#Y^
N>z<v\`
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: W@NM~+)e
x\ieWF1
u|m>h(O
[n/'JeG5
public void wrongStore (int[][] data) throws CloneNotSupportedException{ 19od#
d3+
this.data = (int[][])data.clone(); // Not OK! D3#/*Ky
} %JBFG.+
public void rightStore (int[][] data){ +hdD*}qauC
// OK! |*079v
this.data = (int[][])data.clone(); [t55Kz*cD
for (int i = 0; i < data.length; ++i){ dI!x Ai
if (data != null) :O:Rfmr~
this.data = (int[])data.clone(); DXPiC[g]
} ,: X+NQ
} /{pVYY
eto3dJ!R
9g3J{pKcZ
~YO-GX(
/60`"xH
六、常见错误6#:检查new 操作的结果是否为null k$</7IuH
nenYP0
mG[S"?C
j I
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: iTTe`Zr5y
'0_Z:\ laU
M/GQQG;
olPV"<;+pO
Integer i = new Integer (400); =w HU*mK
if (i == null) 0raVC=[
throw new NullPointerException(); U krqHHpy
W69
-,w/
l,Un7]*
%lZ++?&^
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 @
mm*S:Gt#
loVUB'OSv
eCB(!Y|
a
p-\R
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 $ "[1yQ<p
P+pL2 BA
七、常见错误7#:用== 替代.equals mIVnc`3s
P<b.;Oz__-
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: )'8DK$.
|fdr\t#'~
"Y@rNmBj
`6V-a_8;[
int x = 4; WrhC
q6
int y = 5; +}c
'4hRv
if (x == y) 4,L(
System.out.println ("Hi"); IVD1mk
// This ’if’ test won’t compile. ?Dro)fH1
if (x.equals (y)) 5T,Doxo
System.out.println ("Hi"); gwk$|aT@
%+oWW5q7
)@.bkzW
Tyu]14L
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 REnd#
V2x
_[8xq:G
87%t=X
P9Hv){z
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 ^_b+o
bF %#KSVw
rDkAeX0
lTe}[@(
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 K7}EL|Kx
h: :'s&|
"pq#A*
]#]m_+} Z
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 9v)p0
ul~>eZ
PT4Xr=z =
lJ@2N$w
八、常见错误8#: 混淆原子操作和非原子操作 <tK6+isc
CBx 1.xL
H=]$9ZH!
>nmby|XtW
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: E",s]
5)4*J.
*leQd^47
3/8o)9f.
public class Example{ ^
ab%Mbb
private int value; // More code here... u`Djle
public void set (int x){ k$kq|
// NOTE: No synchronized keyword log{jF
this.value = x; KquHc-fzqr
} ^7v}wpwX\
} Z"#ysC
tr"iluwGc
>XP]NY}Po[
i' J.c4
不过,这个保证仅限于读和写,下面的代码不是线程安全的: kRNr`yfN
1\q(xka{
Sr~zN:wn
(8o~ XL
public void increment (){ S!8eY `C.
// This is effectively two or three instructions: ~Kda#=
// 1) Read current setting of ’value’. `),7*gn*)
// 2) Increment that setting. %Rv&VFg
// 3) Write the new setting back. BDZB;DPb
++this.value; eKn&`\j6
} %)*!(%\S*3
W"4E0!r
{EbR
=
~PaD _W#xP
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: 'qQ 5K
o
e/lfT?J\
@& #df
{U(-cdU{e`
public synchronized void increment (){ r=4'6!
++this.value; t/WauY2JUC
} Y2vzK;
qC?J`
WwbExn<
ntkTrei
]
九、常见错误9#:在catch 块中作清除工作 s<'^
@Y
K"Vv=
A/RHb^N
}MY7<sMDOy
一段在catch块中作清除工作的代码如下所示: #T
Cz$_=t
Nkn0G_
4q[C'
J
E+V^5Z:u
OutputStream os = null; rklr^ e
try{ uSbOGhP
os = new OutputStream (); 9Am&G
// Do something with os here. 4IG=mG)
os.close(); 1g9Qvz3
}catch (Exception e){ W%b<(T;
if (os != null) %1SA!1>j
os.close(); aq~hl7MTj
} 8#'<SB
hXM8`iFW5
-h^FSW($-R
Tn2Z{.q$
尽管这段代码在几个方面都是有问题的,但是在测试中很容易漏掉这个错误。下面列出了这段代码所存在的三个问题: @gENv~m<OI
q7mqzMDk
F6h3M~uR
w{)*'8oCB
1.语句os.close()在两处出现,多此一举,而且会带来维护方面的麻烦。 D|Wekhm
]B=B@UO@.
rZ 9bz}K
Fwyv>U
2.上面的代码仅仅处理了Exception,而没有涉及到Error。但是当try块运行出现了Error,流也应该被关闭。 ^Tc&?\3
6kGIO$xJ)
5+rYk|*D+k
(7`goi7M
3.close()可能会抛出异常。 'IBs/9=ZC
Dk|S`3
K`* 8*k{
cy7GiB2'
上面代码的一个更优版本为: \c/jp5=}
mfFC@~|g
%75|+((fC
znhe]&Fw
OutputStream os = null; ma@ws,H
try{ QR2J;Oj_
os = new OutputStream (); " jn@S-
// Do something with os here. 7oA$aJQ
}finally{ I?dh"*Js&
if (os != null) -VD[iH
os.close(); 8Fx~i#F T
}
FMhwk"4L
*!%y.$\cE
K6~N{:.s
??=CAU%\
这个版本消除了上面所提到的两个问题:代码不再重复,Error也可以被正确处理了。但是没有好的方法来处理第三个问题,也许最好的方法是把close()语句单独放在一个try/catch块中。 /ivt 8Uiw
#9EpQc[4
GV6!`@<
W*;~(hDz
十、常见错误10#: 增加不必要的catch 块 'IP'g,o++
su j? e6
GBtBmV/`
OJ8W'"`L&
一些开发者听到try/catch块这个名字后,就会想当然的以为所有的try块必须要有与之匹配的catch块。 NSHWs%Zc
NLw#b?%
9X,dV7 yW
Y oNg3
C++程序员尤其是会这样想,因为在C++中不存在finally块的概念,而且try块存在的唯一理由只不过是为了与catch块相配对。 T
nAd!
d]VL(&
OgyETSN8C
4pV.R5:
增加不必要的catch块的代码就象下面的样子,捕获到的异常又立即被抛出: 0`"]mYH
pA='(G
vmAMlgZ8{<
`j0T[Pi
try{ Un^QNd>
// Nifty code here !jMa%;/
}catch(Exception e){ H:#b(&qw2
throw e; ?(Dkh${@
}finally{ 4LtFv)i
// Cleanup code here K6@QZc5.!
} =#^%; 6 6z
D^=_408\
L{bcmo\U
Nz#T)MGO`
不必要的catch块被删除后,上面的代码就缩短为: cbsy&U
c 6}d{B[
G5ebb6[+
b=:AFs{
try{ N/DcaHFYo
// Nifty code here qW6a|s0}
}finally{ 9@./=5N~3
// Cleanup code here HC*=E.J
} Kpz>si?CL
;TF(opW:
*GbC`X)
# ,u7lAz
常见错误11#;没有正确实现equals,hashCode,或者clone 等方法 Y"D'|i
+8."z"i3lE
r|:|\"Yk
A`Z!=og=
方法equals,hashCode,和clone 由java.lang.Object提供的缺省实现是正确的。不幸地是,这些缺省实现在大部分时候毫无用处,因此许多类覆盖其中的若干个方法以提供更有用的功能。但是,问题又来了,当继承一个覆盖了若干个这些方法的父类的时候,子类通常也需要覆盖这些方法。在进行代码审查时,应该确保如果父类实现了equals,hashCode,或者clone等方法,那么子类也必须正确。正确的实现equals,hashCode,和clone需要一些技巧。 ]7O)iq%
-2o4v#d
VxLq,$B76
(WR&Vt4R