最近一直在看关于重构的经典:《重构——改善既有代码的设计》,也算是颇有所得。一直想写写感悟,奈何书写的实在太好,感悟的都是里面的原话,实在没必要。昨天同学写了一个简单的小程序,让我帮忙找bug。我一看,觉得这段代码非常适合进行练手,就要过来,花了点时间重构一下。下面进入正题。
程序的目的是,计算两个十六进制数的结果。
输入:
char a[10]= {'0','0','0','0','0','0','0','0','9','1'};
char b[10]= {'0','0','0','0','0','0','0','0','8','7'};
输出:
result[10]:{'0','0','0','0','0','0','0','1','1','8'};
当计算有溢出时提示。
原代码如下:
#include <iostream>
using namespace std;
int hexCharToInt(char c)
{
//十六进制字符转十进制整数,a~f大小写均可
if (c >= '0' && c <= '9') return (c - '0');
if (c >= 'A' && c <= 'F') return (c - 'A' + 10);
if (c >= 'a' && c <= 'f') return (c - 'a' + 10);
return 0;
}
char intToHexChar(int a)
{
//0-15的十进制整数转十六进制字符
if (a > 9)
{
return static_cast<char>('A' + a - 10);
}
return static_cast<char>(a + '1' - 1);
}
char* addHex(char* a,char* b)
{
//两个10位的用数组存储的十六进制数相加
char* result=new char[10];
int intA=0;
int intB=0;
int sum=0;
bool iscarry=false;
for(int i=9; i>=0; i--)
{
intA=hexCharToInt(a[i]);
intB=hexCharToInt(b[i]);
sum=intA+intB;
if(sum>=16)
{
if(iscarry==false)
{
result[i]=intToHexChar(sum-16);
}
else
{
result[i]=intToHexChar(sum-15);
}
iscarry=true;
}
else
{
if(iscarry==false)
{
result[i]=intToHexChar(sum);
}
else
{
result[i]=intToHexChar(sum+1);
}
iscarry=false;
}
}
if(iscarry==true)
{
cout<<"addition overflow"<<endl;
}
return result;
}
int sum(char a,char b){
intA=hexCharToInt(a);
intB=hexCharToInt(b);
return intA+intB;
}
int main()
{
char a[10]= {'F','0','0','0','0','0','0','0','9','1'};
char b[10]= {'1','0','0','0','0','0','0','0','8','7'};
char* r=addHex(a,b);
cout<<r;
return 0;
}
这段代码的“坏味道”是,在addHex函数里,存在Long Method和Data Clumps的问题。而且,很明显条件语句逻辑较为复杂。
首先检查比较简单的东西。在addHex的最后一段,
if(iscarry==true)
{
cout<<"addition overflow"<<endl;
}
return result;
可以通过Extract Method方法提出来作为单独函数:
void checkOverFlow(bool iscarry){
if(iscarry)
{
cout<<"addition overflow"<<endl;
}
}
于是addHex函数就变成了:char* addHex(char* a,char* b)
{
//两个10位的用数组存储的十六进制数相加
char* result=new char[10];
int intA=0;
int intB=0;
int sum=0;
bool iscarry=false;
for(int i=9; i>=0; i--)
{
intA=hexCharToInt(a[i]);
intB=hexCharToInt(b[i]);
sum=intA+intB;
if(sum>=16)
{
if(iscarry==false)
{
result[i]=intToHexChar(sum-16);
}
else
{
result[i]=intToHexChar(sum-15);
}
iscarry=true;
}
else
{
if(iscarry==false)
{
result[i]=intToHexChar(sum);
}
else
{
result[i]=intToHexChar(sum+1);
}
iscarry=false;
}
}
checkOverFlow(iscarry);
return result;
}
下面再看数据泥团。
addHex函数里声明了五个变量:result;intA;intB;sum;iscarry;其中,intA和intB都只在for循环里用了一回,完全没有必要,而且会阻碍对循环体的重构。
可以通过Inline Temp方法消除:找到所有使用到intA和intB的地方,将之修改为对它赋值的表达式:
sum=hexCharToInt(a[i])+hexCharToInt(b[i]);
这样,就可以消除掉intA和intB这两个临时变量了。
另外sum也没有必要在for循环外部声明,将之移到里面来,使for循环体与外面的联系尽可能小:
int sum=hexCharToInt(a[i])+hexCharToInt(b[i]);
这样,addHex函数就变成了:
char* addHex(char* a,char* b)
{
//两个10位的用数组存储的十六进制数相加
char* result=new char[10];
bool iscarry=false;
for(int i=9; i>=0; i--)
{
int sum=hexCharToInt(a[i])+hexCharToInt(b[i]);
if(sum>=16)
{
if(iscarry==false)
{
result[i]=intToHexChar(sum-16);
}
else
{
result[i]=intToHexChar(sum-15);
}
iscarry=true;
}
else
{
if(iscarry==false)
{
result[i]=intToHexChar(sum);
}
else
{
result[i]=intToHexChar(sum+1);
}
iscarry=false;
}
}
checkOverFlow(iscarry);
return result;
}
下面要对for循环里的东西进行操作了。
for循环主要做了两件事:给result[i]赋值,和检查是否需要进位。将这两件事分别提取出函数:
给result赋值:
int getChar(int sum,bool iscarry){
int result;
if(sum>=16)
{
if(iscarry==false)
{
resul=intToHexChar(sum-16);
}
else
{
result=intToHexChar(sum-15);
}
}
else
{
if(iscarry==false)
{
result=intToHexChar(sum);
}
else
{
result=intToHexChar(sum+1);
}
}
return result;
}
检查是否进位:
bool isCarry(int sum){
return (sum>=16)?true:false;
}
addHex函数精简为:
char* addHex(char* a,char* b)
{
//两个10位的用数组存储的十六进制数相加
char* result=new char[10];
bool iscarry=false;
for(int i=9; i>=0; i--)
{
int sum=hexCharToInt(a[i])+hexCharToInt(b[i]);
result[i] = addChar(sum,iscarry);
iscarry = isCarry(sum);
}
checkOverFlow(iscarry);
return result;
}
看起来是不是舒服了许多?
在getChar()函数里,还有一个问题,就是嵌套太多,比较混乱。里面有两个开关:sum和iscarry。怎么减少复杂度呢?
我们观察一下,其实sum和iscarry是有关的,iscarry的含义是进位,它的用处就是将sum加一。因为有了这个关系,我们就可以通过一个表达式来将这两个开关合并为一个:
sum = sum + (iscarry?1:0);
这样,getChar函数就变成了:
int getChar(int sum,bool iscarry){
int result;
sum = sum + (iscarry?1:0);
if(sum>=16)
{
result=intToHexChar(sum-16);
}
else
{
result=intToHexChar(sum);
}
return result;
}
其实if语句还可以精简为:result = (sum>=16)?intToHexChar(sum-16):intToHexChar(sum);
再来一步:result = intToHexChar(sum%16);
这样,getChar函数被进一步精简为:
int getChar(int sum,bool iscarry){
int result;
sum = sum + (iscarry?1:0);
result = intToHexChar(sum%16);
return result;
}
还有一些小的地方,比如在int hexCharToInt()的代码:
if (c >= '0' && c <= '9') return (c - '0');
不符合我们的习惯,一般我们都是用'0'<=c<='9'这样的方式,因此重写为if ('0' <=c && c <= '9') return (c - '0');
最近,完成的代码是这样的:
#include <iostream>
using namespace std;
int getChar(int sum,bool iscarry);
bool isCarry(int sum){
return (sum>=16)?true:false;
}
void checkOverFlow(bool iscarry){
if(iscarry)
{
cout<<"addition overflow"<<endl;
}
}
int hexCharToInt(char c)
{
//十六进制字符转十进制整数,a~f大小写均可
if ('0' <= c && c <= '9') return (c - '0');
if ('A' <= c && c <= 'F') return (c - 'A' + 10);
if ('a' <= c && c <= 'f') return (c - 'a' + 10);
return 0;
}
char intToHexChar(int a)
{
//0-15的十进制整数转十六进制字符
if (a > 9)
{
return static_cast<char>('A' + a - 10);
}
return static_cast<char>(a + '1' - 1);
}
char* addHex(char* a,char* b)
{
//两个10位的用数组存储的十六进制数相加
char* result=new char[10];
bool iscarry=false;
for(int i=9; i>=0; i--)
{
int sum=hexCharToInt(a[i])+hexCharToInt(b[i]);
result[i] = getChar(sum,iscarry);
iscarry = isCarry(sum);
}
checkOverFlow(iscarry);
return result;
}
int getChar(int sum,bool iscarry){
int result;
sum = sum + (iscarry?1:0);
result = intToHexChar(sum%16);
return result;
}
int main()
{
char a[10]= {'0','0','0','0','0','0','0','0','9','1'};
char b[10]= {'0','0','0','0','0','0','0','0','8','7'};
char* r=addHex(a,b);
cout<<r;
return 0;
}
比刚开始的代码好多了吧?一些感想:一定要小步前进,每前进一步都要进行测试,通过了再进行下一步。如果一次改动太大的话,万一哪个地方出现错误,想找出来比较麻烦。还有,函数的名字一定要起的好,根据它的用处来取名。我取的名字还不太好,以后要多注意。
另外,对于如何减少复杂嵌套,我还没有搞定。以后多研究研究。
在重构的过程中,出现一点小问题,在
sum = sum + (iscarry?1:0);
这句话中,我刚开始是这样写的:
sum = sum + iscarry?1:0;
结果出现错误。经检查发现是由于三元操作符的优先级很低,导致先进行了加法,再进行三元操作符运算。
代码是跑在Code Block里的。Code Block是今天第一次用的IDE,感觉很不错,比较轻量,比MS的VS轻多了。好像还跨平台。