代码审查是消灭Bug最重要的方法之一,这些审查在大多数时候都特别奏效。由于代码审查本身所针对的对象,就是俯瞰整个代码在测试过程中的问题和Bug。并且,代码审查对消除一些特别细节的错误大有裨益,尤其是那些能够容易在阅读代码的时候发现的错误,这些错误往往不容易通过机器上的测试识别出来。本文就常见的Java代码中容易出现的问题提出一些建设性建议,以便您在审查代码的过程中注意到这些常见的细节性错误。 $)\ocsO
#nu?b?X'
NDI|;
通常给别人的工作挑错要比找自己的错容易些。别样视角的存在也解释了为什么作者需要编辑,而运动员需要教练的原因。不仅不应当拒绝别人的批评,我们应该欢迎别人来发现并指出我们的编程工作中的不足之处,我们会受益匪浅的。 YxsWY7J
^WVr@6
ci*Z9&eS+
EmH2 Dbw
正规的代码审查(code inspection)是提高代码质量的最强大的技术之一,代码审查?由同事们寻找代码中的错误?所发现的错误与在测试中所发现的错误不同,因此两者的关系是互补的,而非竞争的。 p~M^' k=d
C$?dkmIt
yuhSP{pv'
>S!QvyM(V
如果审查者能够有意识地寻找特定的错误,而不是靠漫无目的的浏览代码来发现错误,那么代码审查的效果会事半功倍。在这篇文章中,我列出了11个Java编程中常见的错误。你可以把这些错误添加到你的代码审查的检查列表(checklist)中,这样在经过代码审查后,你可以确信你的代码中不再存在这类错误了。 hB|LW^@v
rt."P20T
^TDHPBlG
Gh@QR`xxc
一、常见错误1# :多次拷贝字符串 =m1B1St 2
Mb+cXdZb
*%Q!22?6F
-,p(PK
测试所不能发现的一个错误是生成不可变(immutable)对象的多份拷贝。不可变对象是不可改变的,因此不需要拷贝它。最常用的不可变对象是String。 <G#JPt6
a[t2TjB
#,$d!l @
P~=|R9t
如果你必须改变一个String对象的内容,你应该使用StringBuffer。下面的代码会正常工作: CxwZ$0
[['un\~r~
bl:.D~@
EAx@a%
String s = new String ("Text here"); mO]>(^c
,AWN *OS
Li iQ;x
l*/I ;a$
但是,这段代码性能差,而且没有必要这么复杂。你还可以用以下的方式来重写上面的代码: Rl -Sr
qifX7AXHr
`2UzJ~
lS9rgq<n
String temp = "Text here"; $I+QyKO9k
String s = new String (temp); YW0UIO
mmEr2\L
IU/dY`J1
F}.<x5I-;h
但是这段代码包含额外的String,并非完全必要。更好的代码为: BPe5c :z
@)|62Dv /
{_l@ws
%Qrf
]
String s = "Text here"; \h?C
G_|]
Pf-k"7y
&+d>xy\^/
591Syyy
二、常见错误2#: 没有克隆(clone)返回的对象 ,;hIyT
HTvA]-AuM
F3i+t+Jt
gI<TfcC
封装(encapsulation)是面向对象编程的重要概念。不幸的是,Java为不小心打破封装提供了方便??Java允许返回私有数据的引用(reference)。下面的代码揭示了这一点: | Fm(
zT*EpIa+LS
pQ!NhzQ
STL&ZO
import java.awt.Dimension; >@rsh-Z
/***Example class.The x and y values should never*be negative.*/ 6ix8P;;}#
public class Example{ O4S~JE3o
private Dimension d = new Dimension (0, 0); AK} wSXF
public Example (){ } kR3g,P{L
>PGW>W$
/*** Set height and width. Both height and width must be nonnegative * or an exception is thrown.*/ oM M`7wJw
public synchronized void setValues (int height,int width) throws IllegalArgumentException{ Ao69Qn
if (height < 0 || width < 0) /Z94<}C6b
throw new IllegalArgumentException(); ]EC zb/
d.height = height; #ih(I7prH
d.width = width; # 4L[8(+V
} odD^xg"L
J 6D?$
public synchronized Dimension getValues(){ x$tx!%,)/S
// Ooops! Breaks encapsulation 4#1[i|:M
return d; rzsb(
} 6*J`2U9Q
} QX1QYwcm G
q;3.pRw(
;!v2kVuS]
]Nz~4ebB
Example类保证了它所存储的height和width值永远非负数,试图使用setValues()方法来设置负值会触发异常。不幸的是,由于getValues()返回d的引用,而不是d的拷贝,你可以编写如下的破坏性代码: Y!N*J
yl0&|Ub
}1VxMx@
8*-8"It<"
Example ex = new Example();
K[!OfP
Dimension d = ex.getValues(); a{v1[i\
d.height = -5; !o1{. V9q
d.width = -10; d!mtSOh
ze@NqCF
FAkjFgUJp
D~E1hr&Vd>
现在,Example对象拥有负值了!如果getValues() 的调用者永远也不设置返回的Dimension对象的width 和height值,那么仅凭测试是不可能检测到这类的错误。 ?mU
3foa
`$vTGkGpY
CZe0kH^:{
G0Q}
1
不幸的是,随着时间的推移,客户代码可能会改变返回的Dimension对象的值,这个时候,追寻错误的根源是件枯燥且费时的事情,尤其是在多线程环境中。 Ab ,n^
,K'>s<}
J6r"_>)z
Y_tLSOD#/
更好的方式是让getValues()返回拷贝: 1BO$xq
/`McKYIP
FKRO0%M4}Z
Y /$`vgqs
public synchronized Dimension getValues(){ KE! aa&g
return new Dimension (d.x, d.y); e=ZwhRP
} dfl| 6R
. J[2\ "W
ky>0
VO r*YB&
现在,Example对象的内部状态就安全了。调用者可以根据需要改变它所得到的拷贝的状态,但是要修改Example对象的内部状态,必须通过setValues()才可以。 G|MjKe4}
booth}M
WJ":BK{NM
v'gP,UO-%D
三、常见错误3#:不必要的克隆 4gbi?UAmX
VtIPw&KHW
V;0{o
=2!AK[KxX
我们现在知道了get方法应该返回内部数据对象的拷贝,而不是引用。但是,事情没有绝对: |<tZ|
=.NZ{G
9ZFvN*Zf'
l}odW
/*** Example class.The value should never * be negative.*/ ;sJUTp5\h
public class Example{ ]AkHNgW
private Integer i = new Integer (0); >`L)E,=/
public Example (){ }
Z)}q=NjA
%{qJkjG
/*** Set x. x must be nonnegative* or an exception will be thrown*/ LoZ8;VU
public synchronized void setValues (int x) throws IllegalArgumentException{ Xb +)@Y4h
if (x < 0) 4bXAA9"
throw new IllegalArgumentException(); cfI5KLG~#
i = new Integer (x); at7|r\`?-
} v\7k
AdBF$nn[
public synchronized Integer getValue(){ ER,,K._?B
// We can’t clone Integers so we makea copy this way. kB_u U !G
return new Integer (i.intValue()); xHEkmL`)4
} .s$#: ls?
} F jdh&9Zc
"j|}-a
V"":_`1VW
VesW7m*z
这段代码是安全的,但是就象在错误1#那样,又作了多余的工作。Integer对象,就象String对象那样,一旦被创建就是不可变的。因此,返回内部Integer对象,而不是它的拷贝,也是安全的。 Yc;cf%c1
`L0aQ$'>z
s'%KKC
OdX-.FFl
方法getValue()应该被写为: e
iS~*@
g*$
0G
~:7AHK2
W}B4^l
public synchronized Integer getValue(){ _i [.5
// ’i’ is immutable, so it is safe to return it instead of a copy. R<W#.mpo6
return i; "aNl2 T
} ,Ie~zZE&
FI@2KM
.N~qpynY
!NuYx9L?L
Java程序比C++程序包含更多的不可变对象。JDK 所提供的若干不可变类包括: $t5
0<1
TnET1$@qr*
-eAo3
lh`inAt)"
?Boolean 9R.IYnq
?Byte iqf+rBL
?Character 9/C0DDb
?Class Cnf;5/
?Double YN4P
>d
?Float Bv"Fx*{W
?Integer #|*;~:fz
?Long 'qLk"
?Short aq3evm
?String Avw"[~Xd
?大部分的Exception的子类 .UbmU^y|
n`f},.NM|
/1++ 8=
6Nx T W
四、常见错误4# :自编代码来拷贝数组 [:$j<}UmB
BXueOvO8
Jl3l\I'
THQW8 V
Java允许你克隆数组,但是开发者通常会错误地编写如下的代码,问题在于如下的循环用三行做的事情,如果采用Object的clone方法用一行就可以完成: FM9b0qE
S4<@ji
RhyI\(Z2q
9\TvX!)h
public class Example{ ,G1|]
~
private int[] copy; `< y[V
/*** Save a copy of ’data’. ’data’ cannot be null.*/ i2b\`
805
public void saveCopy (int[] data){ !Dkz6B*
copy = new int[data.length]; [$?S9)Xd
for (int i = 0; i < copy.length; ++i) sAk~`(:4!
copy = data; G/(oQA
} BieII$\P%P
} YkPz ~;
tZ>>aiI3
` 2|~Z
H
?t)y/@eG
这段代码是正确的,但却不必要地复杂。saveCopy()的一个更好的实现是: fh$U"
_ %&"4bm.
?>q=Nf^ Q.
z1!ya#,$
void saveCopy (int[] data){ &m8B%9w
try{ doc5;?6
copy = (int[])data.clone(); e^~t52]
}catch (CloneNotSupportedException e){ ~'fa,XZ<
// Can’t get here. f]T1:N*t
} !QVd'e
} sLbz@5 4
i )3Y\u
pRc(>P3;
nIph[Vs-Z
如果你经常克隆数组,编写如下的一个工具方法会是个好主意: T<zonx1
nS?HH6H
b{sE#m%r
+6<MK;
static int[] cloneArray (int[] data){ 9[T#uh!DC
try{ }J`{g/
return(int[])data.clone(); 8SAz,m!W)
}catch(CloneNotSupportedException e){ 58e{WC
// Can’t get here. &[*<>
} e@]cI/j
} od^ha
8GlH)J+kq
~w]1QHA'f
rLL;NTN+/
这样的话,我们的saveCopy看起来就更简洁了: fil6w</L
SVq7qc9K?
w5n>hz_5
E=s`$ A
void saveCopy (int[] data){ \
vJ*3H6
copy = cloneArray ( data); 1}ZBj%z4l
} T82=R@7
RB &s$6A
) 0}o bPp
gP1$#KgU
五、常见错误5#:拷贝错误的数据 u]dpA
`}Q;2 F
Wnf`Rf)1z
RuRJ jcnY
有时候程序员知道必须返回一个拷贝,但是却不小心拷贝了错误的数据。由于仅仅做了部分的数据拷贝工作,下面的代码与程序员的意图有偏差: =Fj:#s
3"L$*toRA
H~V=TEj
:cC`wX$
import java.awt.Dimension; $^
>n@Q@&L
/*** Example class. The height and width values should never * be R2J3R5S=[
negative. */ > %Y#(_~a
public class Example{ sg6cq_\
static final public int TOTAL_VALUES = 10; ,MvvW{EY
private Dimension[] d = new Dimension[TOTAL_VALUES]; IB;y8e,
public Example (){ } [e^i".
`7
B
[<
/*** Set height and width. Both height and width must be nonnegative * or an exception will be thrown. */ }<Me%`x"
public synchronized void setValues (int index, int height, int width) throws IllegalArgumentException{ *M`[YG19!e
if (height < 0 || width < 0) *@BBlkcx
throw new IllegalArgumentException(); iTK1I0
if (d[index] == null) d,0pNav)
d[index] = new Dimension(); 5j}@Of1pd
d[index].height = height; 7olA@;$
d[index].width = width; gTQ6B,`/8
} K
6G n
public synchronized Dimension[] getValues() jS- QTG!=
throws CloneNotSupportedException{ oNh .Zgg
return (Dimension[])d.clone(); x NC>m&T
} ?<}qx`+%Q
} #tfJ?w`
\Q)~'P3
^IId
=V=2
r-0
7!A
这儿的问题在于getValues()方法仅仅克隆了数组,而没有克隆数组中包含的Dimension对象,因此,虽然调用者无法改变内部的数组使其元素指向不同的Dimension对象,但是调用者却可以改变内部的数组元素(也就是Dimension对象)的内容。方法getValues()的更好版本为: @v'<~9vG
V('b|gsEo
w'eenIX^^
#,9|Hr%
public synchronized Dimension[] getValues() throws CloneNotSupportedException{ e,4G:V'NX
Dimension[] copy = (Dimension[])d.clone(); i@;a%$5
for (int i = 0; i < copy.length; ++i){ r\1*N.O3|O
// NOTE: Dimension isn’t cloneable. W>Kwl*Cis"
if (d != null) j>:T)zhyY
copy = new Dimension (d.height, d.width); <;T7qEIlo
} @Uu\x~3y
return copy; PDsLJ|:yL
} Af%?WZlOq
<wA_2S
Y
JU6np 4
t2gjhn^p
在克隆原子类型数据的多维数组的时候,也会犯类似的错误。原子类型包括int,float等。简单的克隆int型的一维数组是正确的,如下所示: 0t&H1xsxX
*~"`&rM(
G@'0vYb#
aD^MoB3
public void store (int[] data) throws CloneNotSupportedException{ t5qAH++axN
this.data = (int[])data.clone(); CG7LF
// OK 7l p VK]
} S.jjB
gdT_kb5HL8
M}<=~/k`j
uj@<_|7
拷贝int型的二维数组更复杂些。Java没有int型的二维数组,因此一个int型的二维数组实际上是一个这样的一维数组:它的类型为int[]。简单的克隆int[][]型的数组会犯与上面例子中getValues()方法第一版本同样的错误,因此应该避免这么做。下面的例子演示了在克隆int型二维数组时错误的和正确的做法: g=(+oK?
LNb![Rq
525 >=h
,+RO 5n
public void wrongStore (int[][] data) throws CloneNotSupportedException{ ?uN(" I
this.data = (int[][])data.clone(); // Not OK! 2I<T<hFW]
} rifxr4c[X>
public void rightStore (int[][] data){ Hn~1x'$
// OK! -T8
gV1*(<
this.data = (int[][])data.clone(); $5IrM7i
for (int i = 0; i < data.length; ++i){ ""d>f4,S
if (data != null) f0s
&9H
this.data = (int[])data.clone(); B@=<'/S\7
} C] w< &o
} Uk9g^\H<D
B`aAvD`7
WdAGZUp
pYG,5+g
\gkajY-?
六、常见错误6#:检查new 操作的结果是否为null )'~FDw\6
A&F4;>dms
aC:l;
Yn>zR I
Java编程新手有时候会检查new操作的结果是否为null。可能的检查代码为: Wwha?W>
JWL J<z
t]
wM_]+
jVIpbG44
Integer i = new Integer (400); (do=o&9pm
if (i == null) ~P_kr'o
throw new NullPointerException(); D8Rmxq!
uN
62>
l`&6W?C
m!60.
检查当然没什么错误,但却不必要,if和throw这两行代码完全是浪费,他们的唯一功用是让整个程序更臃肿,运行更慢。 >EtP^Lu~f_
]F;]<_
|vz;bJG
"}p?pF<'0
C/C++程序员在开始写java程序的时候常常会这么做,这是由于检查C中malloc()的返回结果是必要的,不这样做就可能产生错误。检查C++中new操作的结果可能是一个好的编程行为,这依赖于异常是否被使能(许多编译器允许异常被禁止,在这种情况下new操作失败就会返回null)。在java 中,new 操作不允许返回null,如果真的返回null,很可能是虚拟机崩溃了,这时候即便检查返回结果也无济于事。 ;;!{m(;LS}
o(/ia3
七、常见错误7#:用== 替代.equals 3SDWR@x&
;|QR-m2/
在Java中,有两种方式检查两个数据是否相等:通过使用==操作符,或者使用所有对象都实现的.equals方法。原子类型(int, flosat, char 等)不是对象,因此他们只能使用==操作符,如下所示: *hs<Ez.cC
IGT_
5te
TSSt@xQ+
tq[",&K
int x = 4; Ko)T>8:
int y = 5; *u'`XRJU/
if (x == y) y%bqeo
L~
System.out.println ("Hi"); OIty
]c
// This ’if’ test won’t compile. G@EjWZQ
if (x.equals (y)) x[W]?`W3r~
System.out.println ("Hi"); OW}j4-~wL
h_G|.7!
s4`*0_n
3dDQz#
对象更复杂些,==操作符检查两个引用是否指向同一个对象,而equals方法则实现更专门的相等性检查。 ({kGK0
u6I0<i_KZ
jQeE07g
TL{pc=eBo
更显得混乱的是由java.lang.Object 所提供的缺省的equals方法的实现使用==来简单的判断被比较的两个对象是否为同一个。 OXX(OCG>
gc7:Rb^E5t
Bj($_2M%+
u$,Wyi )L
许多类覆盖了缺省的equals方法以便更有用些,比如String类,它的equals方法检查两个String对象是否包含同样的字符串,而Integer的equals方法检查所包含的int值是否相等。 IOZ|85u=
k,iV$,[TF
V'f5-E0
I u~aTgHX%
大部分时候,在检查两个对象是否相等的时候你应该使用equals方法,而对于原子类型的数据,你用该使用==操作符。 QqK{~I|l
D3AtYt
X:-bAu}D
DD6`k*RIk.
八、常见错误8#: 混淆原子操作和非原子操作 \]#;!6ge
@1-GPmj-
pkV\D
$17
v,
Java保证读和写32位数或者更小的值是原子操作,也就是说可以在一步完成,因而不可能被打断,因此这样的读和写不需要同步。以下的代码是线程安全(thread safe)的: ]#-/i2-K
^_S-s\DW
<_{4-Q>S3#
vQTQS[R=z
public class Example{ M3;v3
}z<-
private int value; // More code here... 19w,'}CGk
public void set (int x){ {9|$%4kRl
// NOTE: No synchronized keyword y7IbE
this.value = x; I1 R\Ts@
} (VXx G/E3
} I%Po/+|+
L!b0y7yR
`ToRkk&&>{
g*r{!:,t
不过,这个保证仅限于读和写,下面的代码不是线程安全的: ~)m t &
JU=\]E@8c
5)n:<U*
N7HbOLpM
public void increment (){ ;'Vipj
// This is effectively two or three instructions: q~'
K9
// 1) Read current setting of ’value’. seqF84Xd<
// 2) Increment that setting. $7gB&T.x
// 3) Write the new setting back. +?5Uy*$
++this.value; Fb{`a[&
} cH* /zNp
~ut& U
o )nT
e"s {_V
在测试的时候,你可能不会捕获到这个错误。首先,测试与线程有关的错误是很难的,而且很耗时间。其次,在有些机器上,这些代码可能会被翻译成一条指令,因此工作正常,只有当在其它的虚拟机上测试的时候这个错误才可能显现。因此最好在开始的时候就正确地同步代码: N} x/&e
6aF'^6+a
pW3)Y5/D
({H+ y
9n
public synchronized void increment (){ peTO-x^a-
++this.value; [>M*_1F
} $G-N0LV
r@ T-Hi
DvRA2(M
Nvx)H(8F
九、常见错误9#:在catch 块中作清除工作 DtJTnvG~B
X~)V )'R
i^s`6:rNu
x,w`OMQ}c
一段在catch块中作清除工作的代码如下所示: (4{ C7
4Fr7jD,#k
z7t'6Fy9'
RLu$$Eb
OutputStream os = null; JJ
N(M*;
try{ sx-Hw4.a"
os = new OutputStream (); =~W0 ~lxX
// Do something with os here. 5}d/8tS
os.close(); K5)yM @cq
}catch (Exception e){ (Xr_ np @
if (os != null) Oj4u!SY\j
os.close(); Q0L@.`~
} <