代码评审三步走-1

上一篇 / 下一篇  2012-07-24 10:05:44 / 个人分类：杂谈

查看( 134 ) / 评论( 0 ) / 评分( 0 / 0 )
51Testing软件测试网(@ rb.uF7n

　　序言

7n~SV"X&Ue051Testing软件测试网(CYr)rD!?!q.`

　　在工作中 参加过不同产品的多次代码检视活动，但在过程中观察到的一些现象引起了我的注意：不管是被检视者还是检视者，似乎都没有什么思路，只是对着代码发呆。即便 能够发现一些问题，多数也是影响甚微的小问题。少数专家虽然能够发现有价值的问题，但也是凭经验+直觉。要问他们怎么做才能发现这些问题，却又没有答案。51Testing软件测试网/H k EA.T9dF

51Testing软件测试网1s0GRxS P

　　为什么会有这样的现象呢？这种情况有没有办法加以改善呢？

;C9[^0}:IOd/^051Testing软件测试网sU%I*M PV:na+r

　　我个人感觉，这是缺乏“套路”所致。研发活动，无论是架构设计，还是代码编写，或者测试活动，都有各自的方法或“套路”。这些“套路”能够指导相关人员有序地开展活动，并保证活动的有效性。那么代码检视是不是也应该有这样的“套路”可循呢？

1nqaH`0

pO3},~7J] p0　　经过一段时间的反复思考，我将自己的一些想法整理成此文，希望能够对代码检视活动提供一些帮助。51Testing软件测试网\`C+\8Dpr.]

51Testing软件测试网O`9x4Z+dO

　　正文51Testing软件测试网F7NB^3OP2B~2@y

A5a4~m h0　　下面将结合示例代码进行代码检视三步走的实战演示。

U t,}j6n7Z5Pl9I7a:R0
51Testing软件测试网K@i HC

bool EnemyChecker::isValidMonster(51Testing软件测试网CQKC n6LR6d*{
NPCManagement::HordeIdType hrdId,51Testing软件测试网5}7_6cWXYa:X
const NPCManagement::MonsterInfoSeq& mnsInfos,
l+x*{)R&`T0NPCManagement::MonsterInfoSeq& vmnsInfos,51Testing软件测试网UP"W(F V3]8g%bb
NPCManagement::FailedMonsterInfoSeq& ivmnsInfos)51Testing软件测试网4gP8Q9K,Y Bm
{
u4T?gm/{qtj0bool bRet = true;51Testing软件测试网3kw){-Xr}3Y J
ivmnsInfos.length(mnsInfos.length());51Testing软件测试网D1NL6m;F'lDjS
vmnsInfos.length(mnsInfos.length());
)lG?7K\0unsigned int iip = 0; // index of invalid horde
p d9yd"RM0unsigned int ivp = 0; // index of valid horde51Testing软件测试网1R,[G2fi!l i
//检查怪物是否属于部落群
7C3Z6Z}G:ehiJ:F0for (CORBA::ULong i = 0; i < mnsInfos.length(); ++i)51Testing软件测试网(|J#k-[(k
{
Dq7D~-W mzQlb ?0const NPCManagement::MonsterStatusInfoSeq& mnsStatusSeq51Testing软件测试网3i/g9m X@M:S8O r
= mnsInfos[i].monsterInfos;51Testing软件测试网RI XF1IyI wv4M+@
for (CORBA::ULong i = 0; i < mnsInfos.length(); ++i)
+vQ+Z0F#O5vH~ C0{
q wMK1FD$H'[0const NPCManagement::MonsterStatusInfoSeq& mnsStatusSeq51Testing软件测试网Xu0c)^ S1^:Z/l
= mnsInfos[i].monsterInfos;51Testing软件测试网&rb3msu}.l
IntSet monsterIds;
4^"jTu @u"fmj0for (CORBA::ULong j = 0; j < mnsStatusSeq.length(); ++j)51Testing软件测试网+ox l9S)|ZV0X0s
{
%FJ;mAU$Fj)xR0monsterIds.insert((int)mnsStatusSeq[j].monsterId);
&Y0w-s+Ft0F0}
;r X3f1|]9Rn0c0if (monsterIds.empty())51Testing软件测试网Q0vFM3d;hVt2Zk
{
RELe-W R0vmnsInfos[ivp].horde = mnsInfos[i].horde;51Testing软件测试网A.G!U&z2J'@ LN1dv
vmnsInfos[ivp].monsterInfos.length(0);51Testing软件测试网3S cj${!L/u
++ivp;51Testing软件测试网*~~i{SQ] d;C
continue;
MD|8Ka9x0}

$z:F;[ x)i051Testing软件测试网H `bb/F

std::map<int, short> mnsTypes;
P)IU @Hx'Fo0NPCManagement::MonsterLoader::getMonsterType(hrdId, -1, monsterIds, mnsTypes);51Testing软件测试网Hc"R2] D|6iVy

51Testing软件测试网*uW)p1E(} pNR

MonsterVec invalidMnsIds;51Testing软件测试网7rliv3yC]0}1}U4g%D
getInvalidMnsIds(mnsStatusSeq, mnsTypes, invalidMnsIds);51Testing软件测试网&nA-vdZ?+BT
//存在部落群和怪物不一致或者怪物类型错误的情况51Testing软件测试网zioqUM kE
if ((monsterIds.size() != mnsTypes.size())
'Zpif dlx3`0|| !invalidMnsIds.empty())
+AI:DK/I0{
u Z{ c1_1jIo X0if (mnsTypes.empty())//一个有效怪物都没有时，记录无效怪物
}(j`W,R'U2t}(f0{
"G3qB b;z%i k0ivmnsInfos[iip].horde = mnsInfos[i].horde;
"y7vl K"vs&e0ivmnsInfos[iip].monsters.length(mnsStatusSeq.length());51Testing软件测试网aYx1P5z Q I
for (CORBA::ULong pos = 0; pos < mnsStatusSeq.length(); pos++)
/Dvu/wL ?(q0{
,k?0s(p*\3^^G5p5sg0ivmnsInfos[iip].monsters[pos].monsterId = mnsStatusSeq[pos].monsterId;51Testing软件测试网.i*Y5w hl8t2f
ivmnsInfos[iip].monsters[pos].monsterName51Testing软件测试网MPAX/puGYu
= mnsStatusSeq[pos].monsterName;
2l W8n^5K0ivmnsInfos[iip].monsters[pos].reason =
%pEn0wI%^ iv0NPC_STR2WSTR(WORLD::Message(getCvt(), "import_invalid_monster"));51Testing软件测试网;q q0o%uQ+{bl
}51Testing软件测试网8DLkfjQ&q)K$ly/L
iip++;
f0L$?"R`L6Gu0}

E!l!_w1A$u L0
else51Testing软件测试网'a_ }oj
{
P4v-h#om|GM]:S0MonsterVec validMonsters;51Testing软件测试网O p&_s w7vD7tH
MonsterVec invalidMonsters;51Testing软件测试网3wFsnj-` inI
validMonsters.reserve(monsterIds.size());51Testing软件测试网R r{#XSRVP
for (IntSet::iterator it = monsterIds.begin(); it != monsterIds.end();++it)
b*G2Yd`x8\a0{51Testing软件测试网 Qiy1~*Ov
MonsterVec::iterator invalidIt
|n:YV8yt)@0= std::find(invalidMnsIds.begin(), invalidMnsIds.end(), *it);51Testing软件测试网#f|:T Y-Ep*X
//数据库中存在，且类型也有效51Testing软件测试网2Hw?~4hR4j1MSb
if ( invalidIt == invalidMnsIds.end()
gG^-^4d*nZ'CTz0&& mnsTypes.find(*it) != mnsTypes.end())
{b(xn(j+e0{51Testing软件测试网3~5f9^|Kaf
validMonsters.push_back(*it);51Testing软件测试网*O\] O };Wd S
}51Testing软件测试网~.N:z jRo6zW
else // 怪物Id和对应的怪物类型不一致或怪物不属于部落群
#? aGGo0{51Testing软件测试网(YA[)`dv&v:t
invalidMonsters.push_back(*it);51Testing软件测试网D@:}:[f2j6g%FR5| V
}51Testing软件测试网ZHhud.w H;L;f^I
}51Testing软件测试网_sB.gW
vmnsInfos[ivp].horde = mnsInfos[i].horde;
/M.Rf5K.U O+o)K0vmnsInfos[ivp].monsterInfos.length((unsigned long)validMonsters.size());
8_5S'w$y|C/Q0for (unsigned int k = 0; k < validMonsters.size(); k++)
2@b7G5Y z@0{51Testing软件测试网 O?z8m}-@ y!gFb
for (CORBA::ULong pos = 0; pos < mnsStatusSeq.length(); pos++)
-},~mp@+BP0{51Testing软件测试网wSl9aRyTDi4}
if ((int)mnsStatusSeq[pos].monsterId == validMonsters[k])51Testing软件测试网${kA*_,@ kI6\tt
{
5W'YQxQ?9U*e^ U0vmnsInfos[ivp].monsterInfos[k] = mnsStatusSeq[pos];51Testing软件测试网Z;Rw!{-{SU
break;51Testing软件测试网`T.D}Q}.D9N@
}51Testing软件测试网&c*e6o^.d:o3kSRV5UX
}51Testing软件测试网 ^Y7MT3V~9bo
}
9qVm{WC/lH!c0ivmnsInfos[iip].horde = mnsInfos[i].horde;
-Ta.u/A }0j$R0ivmnsInfos[iip].monsters.length((unsigned long)invalidMonsters.size());51Testing软件测试网Wg'I XMi Hl
for (unsigned int k = 0; k < invalidMonsters.size(); k++)
(f3S2L[\@$n0{
t9Oe/s{Jc)tA+N0for (CORBA::ULong pos = 0; pos < mnsStatusSeq.length(); pos++)51Testing软件测试网8evXe L9}
{
ur O2N_0oZ Ya0if ((int)mnsStatusSeq[pos].monsterId == invalidMonsters[k])51Testing软件测试网Lr8s8C.m6A| k
{
rVZ;Sh,T p0ivmnsInfos[iip].monsters[k].monsterId = (unsignedint)invalidMonsters[k];
P6]7xf `(l D%{'qSY0ivmnsInfos[iip].monsters[k].monsterName =51Testing软件测试网V"N&AmZ
mnsStatusSeq[pos].monsterName;51Testing软件测试网4N-BG$K'G d2H0i*daK
ivmnsInfos[iip].monsters[k].reason =
;Z`X"E?0NPC_STR2WSTR(WORLD::Message(getCvt(), "import_invalid_monster"));51Testing软件测试网!Y)c8V2{-co(~R
break;51Testing软件测试网 a0U{&|Z:[.a
}51Testing软件测试网:JBu McN8C&F)O
}51Testing软件测试网N,T6i Fz(G
}
{xAn0aS0xb1m0ivp++;
,E]"}#xb0iip++;
-P_I%\5{:gZ5XR0}51Testing软件测试网%p)b"S'b.a ?O3dE
}
\pR| W4zj0else//部落群和怪物一致，且对应怪物类型一致
[;|;UkfF&H?0{
-oxjgW0vmnsInfos[ivp].horde = mnsInfos[i].horde;51Testing软件测试网%P,t:@0j9}:tU(e
vmnsInfos[ivp].monsterInfos = mnsStatusSeq;51Testing软件测试网kO+d`&SY1@
ivp++;51Testing软件测试网nnp ? a}^ ]
}51Testing软件测试网V4V@J^^r D9KA
}51Testing软件测试网@b,xR7x^)ac+`c Q
ivmnsInfos.length(iip);51Testing软件测试网X,}8tx U!N
vmnsInfos.length(ivp);51Testing软件测试网1M'~X\Ge
return bRet;51Testing软件测试网qPfkT f'?n
}51Testing软件测试网c5^/?$TzF S!c

　　示例代码段一：

D5Dv(Y\M*i051Testing软件测试网2d n;NfK:d

　　第一步：51Testing软件测试网 N D+lg+_JG

51Testing软件测试网S \t I7Qcno

　　代码检视要做的首先就是评审代码的逻辑正确性，即代码是否完整、准确地实现了业务逻辑。51Testing软件测试网l h:ex9}+xu

51Testing软件测试网!d H&Oa!L&V.z E']OE

　　这一步只需对代码进行抽象，将其“翻译”为“注释”就可阅读其实现逻辑，并与需求逻辑（设计逻辑）相比较。

gC I"{j1X+C F^(r051Testing软件测试网 R*s'j3n"N3[;S@m%U

　　这第一步很重要，而且实践难度其实不高。对于有“注释驱动编码”习惯的开发人员来说更是容易。

1}.NN3Yw"c;MIa051Testing软件测试网/~R#w0C,Z [m

　　对示例代码段一，“翻译”成简略的“注释”（有省略），可以如下文（以大括号{之后紧接的为第一行）：

)~1B|)V|U,M(N f@E(bE }051Testing软件测试网 E9q`Kzh}

51Testing软件测试网wWN3yG

//（1-5行）初始化输出值51Testing软件测试网Z-L0|-~.a$i}E&dX
//（6-102行）超大循环，遍历输入的mnsInfos，对每一项执行以下的操作：51Testing软件测试网^6mae&up_%K'}*o:F
//（8-14行）首先从mnsInfos中取出一组数据并记录其中的所有怪物ID51Testing软件测试网K ]&t Y+e5]1O5q z
//（15-21行）如果该组数据为空，则记入输出参数vmnsInfos并跳过
:XI3RVQ(E_(~0//（22-25行）（如果该组数据不为空）根据这一组怪物ID获取其对应的怪物类型（mnsTypes）及无效怪物的集合
/S R c+|k0x-W~1S%d0//（26-95行）如果存在部落群和怪物不一致或者怪物类型错误的情况，那么：51Testing软件测试网7PFr"` [@ c
// a、（29-42行）若一个有效怪物都没有时，记录无效怪物
1i%EYTu0// b、（43-94行）否则遍历这一组所有怪物，并分别记录下有效值和无效值，当
(NE:~bt*T7[0//1、数据库中存在怪物信息且类型也有效时，记为有效怪物，并存入输出参数vmnsInfos51Testing软件测试网!E _`@yD^
//2、怪物ID和对应的怪物类型不一致或怪物不属于部落群，记为无效怪物，并存入输出参数ivmnsInfos
Chudpx \0//（96-101行）否则如果部落群和怪物一致，且对应的怪物类型也一致，则将信息都存入vmnsInfos
m4ODqzfE7?0\5cC0//（103-105行）循环结束，返回执行成功标识

%I`.Y5rfY4Z0　　从以上的“注释”中，可以看出几点：

"NO'f @/X d051Testing软件测试网M Y1sfu r(ydv

　　1、虽然该函数有100多行，但是其实逻辑并没有那么复杂。51Testing软件测试网)wT X'H&C!CM0q"F3F ZA

51Testing软件测试网)e%u7h;z5G7NQQ"SRT

　　2、逻辑并不复杂的代码却写出100多行，其中大量代码都是在做基本的sequence、vector遍历、拷贝的原始动作，这些完全可以剥离出去，使主体逻辑清晰化。

/r-s*n0a6XLw0

q \&g `%OM-k0　　2、返回值不能指示任何情况，因为只要函数执行完就必定返回true；如果中途抛出异常，该函数也没处理，自然也不会有返回值。该设计不是很合理。

m*wn)ii%JvH0

%oJ9~sH0　　如果有足够的把握，上面的“翻译”过程也可以在脑中进行，无需写出来。51Testing软件测试网9U*e1P [i A;E

*H#j L]E)[0　　当代码“翻译”为“注释”后，就可将提取出的逻辑与设计和需求进行比对，检查其是否忠实体现了设计，是否完整、准确地实现了需求。51Testing软件测试网sNvZ |^VA

i'H-J d pd ZbJ0　　需求和设计文档这里就不给出了，对抽取出的逻辑参考相关文档进行检查不是太困难的事，在此不展开。

9@\3sqv.] c$N051Testing软件测试网%V4X?$D8N8b

　　第二步：51Testing软件测试网W&eap7Kr:a o

51Testing软件测试网7[6H9n@.d:B#W

　　接下来要做的就是针对“常规输入”的可能取值进行核查。所谓“常规输入”就是指函数的参数，这是函数的显式输入。51Testing软件测试网0k0l(^t1S6L

So t'W0g0　　我们知道，任何一段程序都可以用IPO（Input-Process-Output）模型来分析、检查。所以代码检视要做的也是针对所有输入的可能取值，检查代码段是否有正确的处理，能否产生正确的输出。51Testing软件测试网2u#d H!P3T2|Z `"{

51Testing软件测试网ozn4z p SHXA:Zi

　　函数参数正是最显眼明白的输入项，对它的取值采取等价类划分和取边界值，就能够检视出代码中是否能够覆盖到各类的输入组合。51Testing软件测试网xh1^0rA8k.NL3Do

51Testing软件测试网$u6w md0\

　　示例代码段一中，函数有两个输入参数hrdId和mnsInfos（余下两个是输出参数），我们一个个地看。51Testing软件测试网,e |N*?+N

I%w \W6eE&U0　　hrdId在代码中仅有一处使用，即51Testing软件测试网A(X:h Z\3u

51Testing软件测试网^.E^"^&HH

51Testing软件测试网aswQDmQ

NPCManagement::MonsterLoader::getMonsterType(hrdId, -1, monsterIds, mnsTypes);

2k2S"Ci1x b GSC(o0　　这是一个外部调用。

+[1i#k3QV;\+V1g0

:u Z,JT `/D^-~0　　第一条、检视规则：输入参数仅用于外部调用的情况，在代码检视中可不必关注。

Z!n+Lk!J\2D l0

?2z'l)\'@U"q/sD3n0　　因为通常情况下这样的输入参数不影响被检视函数走向，而是否影响被调用的外部函数、对象，则是应当在其他的检视活动中覆盖的内容。

%Nh \W~0

'_z`.@c&M9\3K0　　该规则，我们在“常规输入”检查阶段跳过这一行。

%k"tFL T i0@*`0

收藏 举报

TAG:

 

评分：0

我来说两句

Open Toolbar