你的代码真的很健壮吗
上一篇 / 下一篇 2007-11-02 10:49:29 / 个人分类:你的代码要跟男人的身体一样健壮!
在编写对话框程序的时候,我们时常会需要Enable或Disable某个控件,有些追求代码健壮的程序员会写出这样的代码:
.w]H9RoYm0
QKq-~x*wB0
| void CMyDialog::OnStart() m fB8oA F9~[zwB0{测试爱好者:wsx}BC Kx@l/Q CWnd* pBtn = GetDlgItem( IDC_ADD_BTN);测试爱好者-J0QTk#e2Hn if( pBtn ){ iA.`c8FTJK5F0 pBtn->EnableWindow(FALSE);测试爱好者R _F4u|,} } D.aI%t2M1nN x^0 …测试爱好者9n:[_!ST^:? } $\!lr.S] qO0测试爱好者)x CbHa4\0y'F void CMyDialog::OnAdd() V@9d8}"c0{测试爱好者v ?|V@ ......测试爱好者F1[ER9QK}@ } |
-|%UYi4V0 由于GetDlgItem()返回的是一个CWnd的指针,按照文档的描述,如果指定的控件不存在,该函数会返回一个NULL指针,为了确保不会调用NULL指针的函数,我们先检查了返回的指针是否为NULL。
+PBx ] gfhW!b0测试爱好者 k{"Gu\4R5i9t1D?
一切看上去很美,这段程序永远不会使你的程序崩溃。然而,不会崩溃的程序,不一定是没有问题的程序。假设在MyDialog中Add按钮被定义成IDC_BTN_ADD,并且不凑巧,在这个项目的另一个Dialog里也有一个Add按钮,而且它的ID被定义为IDC_ADD_BTN,所以你的程序在编译和连结时都不会有错误。当用户使用时,也不会注意到有什么不妥,只是Dialog上的某个按钮没有变成灰色,没有人会注意到它的。测试爱好者 u6C5\"R(M;s
测试爱好者(@2}$ze?V`
然而,它并不符合你的设计,也许在程序的其他地方,你假设在任务开始后,OnAdd()函数不会被调用到。这些问题一直隐藏着,直到有一天,用户报告说按Add按钮,加入某些数据后,按Ok,结果程序崩溃了。你在自己的机器上试了一下,由于之前你没有按过Start按钮,所以你一直复制不出这个问题。经过几个来回的email或者电话交流,你找到了复制错误的方法,并且奇怪为什么Add按钮没有被禁止呢,奇怪??忙活半天后,你发现原来是一个ID写错了。测试爱好者 s/MC[_)hm+d
8s'bi(xUD;|9G]3LJ0 一个很小的错误,修正它也许只要两分钟,找到它却花费了你几十分钟甚至更长。然而,这一切是可以避免的。这里我们要避免的不是说写错ID,粗心大意的错误,人人都会犯,而且会不停的犯。但是如果错误能够被及时发现,就会剩下许多时间。测试爱好者"V#s,`P5GN6g
c,f|7n(|BE0 造成以上问题的原因是我们在代码中加入了一些防御性的代码,这些代码保护了程序员犯的错误。如果GetDlgItem()返回NULL,一定是由于程序员的错误。由于错误被掩盖起来,所以当问题被暴露出来时就已经面目全非了。
GEGx&M0测试爱好者\$yMI u*{H{
一个比较好的做法是除去防御性代码,让问题及早暴露:测试爱好者ky$JdUy1~I(C
测试爱好者Z$zx.@A\d
| void CMyDialog::OnStart() (MgJ:XO z-E@0{测试爱好者G1A*qH'?9F.E GetDlgItem( IDC_ADD_BTN)->EnableWindow(FALSE);测试爱好者AQT"Tqezh| …测试爱好者^"m.I}X } |
这样的结果是:一按Start按钮,程序立刻就崩溃了。的确,崩溃是很严重的错误,在Bug List里它的优先级是比较高的(仅次于造成整个OS崩溃)。但是,既然有错误,迟早要崩溃的,还不如早一点崩溃。至少早一点崩溃可以使你很快就发现问题,找到问题。有经验的程序员都清楚,一触即发的问题并不可怕,可怕的是那些偶然发生,不容易复制的问题。测试爱好者GQLG0d-S
测试爱好者.B;O'U"st1X#W'{
需要在函数里检查参数的合法性吗?测试爱好者MYb.q5R@%X
测试爱好者;p"B2nO0D[2K`E
在实现一个函数时,出于“健壮性”的考虑,我们经常会在函数的入口处加入许多参数检查代码。比如以下的一个例子:测试爱好者?i A&F6F
u*`V4O#X;e K*N0
| class CItemManager测试爱好者/L RyT"p'} { \5SzGF0 protected:测试爱好者{"Zi3?'U(G int m_nCount; 6Ft?6dj:xV7u4C8x0 … g0Bk*y5w]QGZ0 public:测试爱好者u.ZFUH"c5Qo int GetItemCount();测试爱好者g9WG4P kzH%n CItem* GetItem( int nIndex );测试爱好者2hgoL+~'a }; ME;^*[F}0测试爱好者Hq2G4iD'XGmp CItem* CItemManager::GetItem( int nIndex )测试爱好者 ~yB0F^c*g~S,b7H {测试爱好者XH\,J%?0P9k if( nIndex < 0 || nIndex >= m_nCount ) Z;PX$s-z#L0 return NULL; .eP2K1o$s.Cs5Kf0 …测试爱好者+Aj3} o/v(m return pItem;测试爱好者IuZ!n$\q[:ew }测试爱好者*tBWE2H 测试爱好者,|N0Mp&O)kE class CItemManager测试爱好者o??7_#aFp {测试爱好者8Wyjgvd/]B8{ protected:测试爱好者)vb#_w+F(y?5W)a8Zey int m_nCount; +t&o1fBv,w0 …测试爱好者M9E1q[4q.Mu&s public: w9}:tMT-dkDze [0 int GetItemCount(); ;ncqYwG0 CItem* GetItem( int nIndex );测试爱好者1D8}|y}g };测试爱好者0U2h3p;N-A|,C )es~ l HI0CItem* CItemManager::GetItem( int nIndex ) y^X*oy0{ :f.c3A3U\su"U&@0 if( nIndex < 0 || nIndex >= m_nCount ) +gj!g Icc0 return NULL;测试爱好者rI-T5A U'z!v0W … ?vsLI.f0 return pItem;测试爱好者7zO1])h?t:wa } |
P-\$iS8`$c(o,Rz0 在实现GetItem()时,你首先检查了参数的合法性,如果不合法就返回一个NULL指针。这样你的函数在任何的输入情况下都不会导致程序崩溃,一切看上去完美无缺,无可挑剔。但是,这样做真的能使我们的程序更健壮吗?
%E HW#dOQ)Kmd9G0
O:e+Y)] |5e0xk/@0 我们从调用者的角度来分析一下。为什么调用者会传入一个不合法的参数呢?一种情况是调用者的程序有bug;另一种情况是调用者不确定index是不是合法,但是他不想多写两行代码来判断index的合法性,他希望GetItem()能够一次都给办了:即能检查index的合法性,又能返回CItem的指针。
3{nj7{N,]3q#p/@/w0测试爱好者_0z&CLkU
考虑第一种情况,也许调用者写了如下的代码:测试爱好者p7NCh`:M9H,vYU
测试爱好者Sqw!qQ
| int index; 6JhzC&]2oGP0…测试爱好者,Ee.t1w!y vm-n M7j%}0CItem* pItem = im.GetItem( index ); .WH8F7R$W;G']*G/N0if( pItem ){//should be executed测试爱好者%~]P\ [ Q.`0} … d-N!M)eiw)h+@jq#p0} |
这是一段危险的函数,index变量在使用之前没有初始化,但是这段程序不会,永远也不会使程序崩溃,这要感谢实现CItemManager和使用CItemManager的程序员,他们都习惯于写“健壮的”代码。但是,这段程序却不会按照我们想象的那样运行。本该执行的代码并不是每次都被执行到,因为谁也不确定index变量里存的是什么东西。这段代码是健壮的,他不会使程序崩溃,但是程序的运行过程却是不确定的。一旦出现问题,这个问题即不容易复制,也不容易确定错误原因,它的表现形式往往出乎你的意料。
R F#|^xZY0
e)]m4K C'R?{!{ Df0 考虑第二种情况,调用者通过其他函数得到了一个index,然后他想取得这个index下的CItem指针,但是他不想多写两行代码去判断这个index的合法性,他想:“如果这个index是合法的就请返回给我这个index下的CItem指针,如果不合法就返回一个NULL好了“。这样做只要一行代码就够了,他省下了一行代码,也许不止一行,因为在很多地方都需要呼叫GetItem()这个函数,所以,他省下了许多行代码。他会这样使用GetItem():测试爱好者4Oo7P ?/sQU@BX
b(N6vY0K3U+U3R0
| void SomeFunc( int nIndex )测试爱好者'?A*_
K0@ nw {测试爱好者 KS6[ z4];H;wl:k CItem* pItem = im.GetItem( nIndex ); Qpn2V)J [0 if( pItem ){测试爱好者 hP;A)E H //do something.测试爱好者8k;Gy{:_`B._ |Y }测试爱好者SHEO;u"DyI } ^LDbG-dH0测试爱好者&NSv h!z#?~2K void SomeFunc( int nIndex )测试爱好者!d si2Css { _j7c)t9rps w0 CItem* pItem = im.GetItem( nIndex );测试爱好者 {FedI[k;e if( pItem ){测试爱好者` B:M:eA-g i //do something.测试爱好者r[#k+Mo? } h,zxx| BS0} |
如果CItemManager的实现者和使用者是同一个程序员,我们经常会写出像上面的代码,毕竟可以省下一行代码,而且上面的代码看去还不错,简洁明了。但是仔细推敲一下, 我们发现,按照以上的要求实现的GetItem()是一个不良的设计。测试爱好者1uB'y4X!f4l
测试爱好者~'`3i!_0q.C$Wfw0BB
首先,它违反了单一职责原则(SRP)。按照以上的要求实现的GetItem()其实完成了两项功能:第一项功能用来判断index是否合法;第二项功能用来取得指定index下的CItem指针。GetItem()只应该负责取得指定index下的CItem指针,检查index的合法性应该交给其他的函数。这里,调用者可以通过GetItemCount()来判断index的合法性。测试爱好者`Ndbi h4P
测试爱好者In3[1P9s"[Cf5a
其次,函数的返回值具有二义性。如果函数返回NULL,那么这个NULL可以代表index不合法,也可以解释为,指定的index下的值就是NULL,因为从编译器的角度NULL也是一个CItem指针。这里GetItem()混合了两种功能的返回值,而且第一项功能的返回值使用了第二项功能的返回值中的一个特例。这样的设计破坏了程序的完整性。假如CItemManager不是管理CItem指针,而是管理CItem对象,你就不会那样设计GetItem()函数了。测试爱好者+bL2JnM~
测试爱好者T"S7x+JG
如果真的想在GetItem()里实现index的合法性检查,那么GetItem()的定义应该改成这样:
U.o|'u;{~ B FpN0测试爱好者 GR5B?Z})C2m'A
| bool GetItem( int index, CItem* & pItem ); |
D7KRV(R8b P;A3q0 如果index不合法,函数返回false;如果index合法,函数返回true,并且pItem返回该index下的CItem指针。经过这么一改,返回值的二义性被消除了,但是你是否觉得,GetItem()的语义已经有点变味了,这更像是在实现FindItem了。然而,按照index去Find一个Item似乎又不合理,我们进入了一个两难的境地。测试爱好者m5IZ"N5A(i9m}b
测试爱好者x A$K TY;C-V
退一步海阔天空。在GetItem()里检查index的合法性,并不会让我们的程序更健壮。一个比较好的做法是,由调用者负责index的合法性检查。测试爱好者/}+{Le,n ez:b
测试爱好者J&|0E/x+a,ai$iq q9z
所以 SomeFunc应该改成这样:测试爱好者%N.W he~4A)l}
%E\x zX.zX0
| void SomeFunc( int nIndex ) ;Z&Y!A F$G+W2i}0{测试爱好者*J xP*b+\i if( nIndex >= im.GetItemCount( ) ) !szs.a6G n'Hq;j0 return;测试爱好者9{ak4^#oc"J m{"k CItem* pItem = im.GetItem( nIndex );测试爱好者gWWEFD9i pItem->…; O|L~?P$HLk4m0} ;c L$d%TS"Lib0f0测试爱好者Q){,p^oO q void SomeFunc( int nIndex )测试爱好者G)Wj6I/I!c)o {测试爱好者CH(o;R-GILr/xT if( nIndex >= im.GetItemCount( ) )测试爱好者7D0n1J?0i~ return; 7u+Q9j`(j#Xi0CItem* pItem = im.GetItem( nIndex );测试爱好者Z9\:n0ddM pItem->…; 7V{NZ#W4m*]R0} |
oQ J7d%Jg(R0 而GetItem()的实现应该改成这样:
![%m]+E9\5Nb Pn0
hgZczw0
| CItem* CItemManager::GetItem( int nIndex )测试爱好者,SH!l`f8hu { -AITEc0 ASSERT( nIndex >= 0 && nIndex < m_nCount );测试爱好者*K|$dy}N0_4B}Y E4@ … %g9QS Z:HBK-m0 return pItem;测试爱好者6f5w sVQ x } |
|+k$?%?hru cI0 以上的实现我们使用了ASSERT()来检查参数的合法性,当参数不合法时,程序会被终止。ASSERT()断言只有在调试版才有效,所以程序不能依赖它来做错误处理。ASSERT()在这里的作用是,一方面在调试程序的时候,能够帮助我们尽早的发现错误。错误越早被发现,越容易被解决;另一方面,按照Robert Martin在Agile Software Development一书中所述,软件具有三项职责,最后一项便是:和阅读它的人沟通1。这些断言代码可以向阅读代码的人传递这样的信息,当程序运行到这里的时候,必须满足这些条件。测试爱好者S!z`;rT/w A`
*zP,U;Hee0{)?K8W+V0 我是否在鼓励不要写防御性代码?测试爱好者0Q'ya#_IA
4N)d7v(R{m)v:j0 读到这里,你也许觉得我在鼓励不要在函数里检查参数的合法性,不要写防御性代码。是这样吗?答案显然是否定的。我要强调的是不要盲目的加入防御性代码,这样做并不能增强系统的健壮性。当要加入防御性代码的时候,你需要
`({G4xe.lG8]3Z0测试爱好者Jens+F8o kvb
分析一下,这个条件是应该假设的,还是应该防御的。对于应该假设的条件,可以使用ASSERT()断言来检查,对于应该防御的条件,必须用专门的代码来处理。测试爱好者8d1h)T2Lq.YZ_&i
jA^-X9_0 那么如何判断一个条件是应该假设的,还是应该防御的呢?这让我想起了荣耀先生(optimizer)的两篇沉思录,《你防御了吗?》2和《别人的棺材》3。测试爱好者!R%j#HV_MZ3@5R M
I#D+J;n"D2WS0 《你防御了吗?》说的是作者写的一个用于显示SQL语句的程序,作者假设输入的SQL语句不会超过4000个字符,结果有一天的确有人输入了超过4000个字符的SQL语句,然后程序崩溃了。这引起了作者对防御性编程的思考。