重构实践——重构的小例子

最近一直在看关于重构的经典:《重构——改善既有代码的设计》,也算是颇有所得。一直想写写感悟,奈何书写的实在太好,感悟的都是里面的原话,实在没必要。昨天同学写了一个简单的小程序,让我帮忙找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轻多了。好像还跨平台。














                
评论
添加红包

请填写红包祝福语或标题

红包个数最小为10个

红包金额最低5元

当前余额3.43前往充值 >
需支付:10.00
成就一亿技术人!
领取后你会自动成为博主和红包主的粉丝 规则
hope_wisdom
发出的红包
实付
使用余额支付
点击重新获取
扫码支付
钱包余额 0

抵扣说明:

1.余额是钱包充值的虚拟货币,按照1:1的比例进行支付金额的抵扣。
2.余额无法直接购买下载,可以购买VIP、付费专栏及课程。

余额充值