代码优化重构(很经典)

来源：互联网发布：电子卷宗网络查阅编辑：程序博客网时间：2024/04/23 14:47

1、清晰地表达意图

2、一个方法只做一件事情

3、同一个方法体内，保持相同的抽象层次

4、不要重复自己（避免手动的复制与粘贴代码）

5、减少“语法噪音”

6、命名时取有意义的名字，避免不规范的缩写

publicDataSetGetDefectDetails(stringsDefGrp)

{

bool bResult =false;

DataSet dsData =new DataSet();

try

{

HashtablehtFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

bResult = fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

}

catch(Exception Ex)

{

stringsErrorMessage = Helper.GetErrorDescription(ex.Message);

if (sErrorMessage== string.Empty)

{

sErrorMessage = ex.Message;

}

Master.ErrorMessage = sErrorMessage;

m_logMsg.Message = "GetDefectDetails : " +sErrorMessage;

m_logger.LogMsg(m_logMsg);

}

returndsData;

}

我们来看看，这20行代码，真正“在干活”的代码有哪些？

看来看去，只有这么一行：

bResult =fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

我们可以说，其他的代码，其实都算是“语法噪音”。当然，由于我们不是生活在真空中，所以有些语法噪音是没办法完全避免的，我们能做的只是“减少”，使之不至于影响我们意图的表达。

但是，单单分析这个语句，我们也能发现一些“噪音”，这里将fisService.GetDataSet的结果赋给变量bResult，如果不看完整个方法，我们会假定这个bResult就是这个GetDefectDetails方法的返回值，但看到最后我们惊讶地发现，返回值是dsData，而这个bResult居然没用到。

可以推断，这就是复制粘贴代码惹的祸，在原来复制代码的地方，bResult应该就是方法的返回值，在copy过来后，由于这个变量不影响执行的过程，就被保留下来了。我们前面说过，代码有双重目的，copy代码的人显然忽略了第2个目的：代码的可读性。

OK，我们修改一下，这段代码可以变成19行，而且我们也不用再关心那个奇怪的变量bResult：

publicDataSetGetDefectDetails(stringsDefGrp)

{

DataSet dsData =new DataSet();

try

{

HashtablehtFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

}

catch(Exception Ex)

{

stringsErrorMessage = Helper.GetErrorDescription(ex.Message);

if (sErrorMessage== string.Empty)

{

sErrorMessage = ex.Message;

}

Master.ErrorMessage = sErrorMessage;

m_logMsg.Message = "GetDefectDetails : " +sErrorMessage;

m_logger.LogMsg(m_logMsg);

}

returndsData;

}

有同学说，这个改善太微不足道了。对的，重构的精神就是小步推进，勿因善小而不为，通过不断积累，最后就可以得到一个清晰、易维护的系统。

然后，我们的目光移向那个可疑的if语句，为什么要做这个判断？或者说，这个判断能不能放到Helper.GetErrorDescription方法内部？通过阅读Helper.GetErrorDescription方法的代码，我们发现这个判断完全可以方法内部。

在这里，造成重复代码的原因是：写共用方法的人未考虑周全，导致客户需要关心方法的条件分支情况，从外部进行判断。

通过将判断逻辑移到Helper.GetErrorDescription方法内部，代码变成15行。

publicDataSetGetDefectDetails(stringsDefGrp)

{

DataSet dsData =new DataSet();

try

{

HashtablehtFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

}

catch(Exception Ex)

{

stringsErrorMessage = Helper.GetErrorDescription(ex.Message);

Master.ErrorMessage = sErrorMessage;

m_logMsg.Message = "GetDefectDetails : " +sErrorMessage;

m_logger.LogMsg(m_logMsg);

}

returndsData;

}

现在再看看，try…catch…的语句块还是占据了大部分的空间，而且，每个异常处理的过程都是一样的，这里也会导致大量的手动复制粘贴代码，难道这部分代码不能只写一次吗？我们重新审视一下代码中变化的地方，用高亮显示出来：

try

{

Hashtable htFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

}

catch(Exception Ex)

{

stringsErrorMessage = Helper.GetErrorDescription(ex.Message);

Master.ErrorMessage = sErrorMessage;

m_logMsg.Message = "GetDefectDetails : " +sErrorMessage;

m_logger.LogMsg(m_logMsg);

}

OK，看来我们需要一个模板方法，第1个高亮的语句我们可以定义成一个委托，第2个高亮的语句是其实是方法的名称。我们可以在基类中定义：

protecteddelegate void MyAction();

protectedvoid HandleException(MyAction myAction)

{

try

{

myAction();

}

catch(Exception Ex)

{

stringsErrorMessage = Helper.GetErrorDescription(Ex.Message);

Label lblMsg =(Label)Master.FindControl("lblMsg");

if(lblMsg != null)

{

lblMsg.Text = sErrorMessage;

lblMsg.CssClass = "Err";

}

mylogMsg.Message = myAction.Method.Name + " : " +sErrorMessage;

mylogger.LogMsg(mylogMsg);

}

好了，这回我们连处理异常的细节都不用关心了。由于.net2.0可以写匿名方法，代码可以写得很简洁，新的写法只剩下8行：

publicDataSetGetDefectDetails(stringsDefGrp)

{

DataSet dsData =new DataSet();

HandleException(delegate()

{

HashtablehtFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,ref dsData);

});

returndsData;

}

好了，现在看看我们的成果：

1、把代码从20行简化到8行（减少语法噪音）

2、将重复的逻辑放到公用方法中（避免手动的复制粘贴代码）

3、需要关注的地方大大减小（不用再关心bResult变量和异常处理细节，更专注于业务逻辑）

在现有框架的基础上，这已经算是一个极致，下面，让我们作一个小小的冒险，假设框架可以由我们自由定义，看看我们可以重构到什么地步。

首先，通过AOP(aspect-orientedprogramming)框架，我们可以让异常处理语句完全在方法体内消失，只需要在处理异常的方法上打上一个标签：

[HandleException]

publicDataSetGetDefectDetails(stringsDefGrp)

{

DataSet dsData =new DataSet();

HashtablehtFilters = new Hashtable();

htFilters.Add("GROUP_NAME", sDefGrp);

fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters,refdsData);

returndsData;

}

这里体现了“声明式编程”的风格，即只要说明意图，而不需要写出处理细节。

当然以上仅为示意写法，HandleException的attribute必须另外定义，使用不同的AOP框架，写法也有不同，常用的AOP框架有Sprint.net, Castle,PostSharp等。

剩下最大的“噪音”应该就是这个Hashtable了。在这里定义Hashtable，仅仅是框架的需要，同样，QueryName也是框架的需要，如果按“正常的写法”，代码应该是：

[HandleException]

publicDataSetGetDefectDetails(stringsDefGrp)

{

returnfisService.GetCodeByGroupName(sDefGrp);

}

OK，这回这个方法终于露出本来面目了，GetDefectDetails方法根本没有处理任何业务逻辑，之所以有20行的代码，只是因为框架本身不够完善，无法帮助程序员简化开发而已，所以…其实…这个方法…可以拿掉…

GetDefectDetails方法：哦，不！你不是来帮我改进的吗？怎么把我给杀掉了？

背景声音：不好意思，我国的改造，历来如此…

余论：

1、问：如果方法删掉了，在哪里注明需要异常处理？

答：可以在调用GetDefectDetails方法的方法上打上HandleException的attribute，或者在配置文件中指定webForm中所有的异常使用统一的方法处理。

2、其实我理想中的方法签名应该是：

Class CodeService…

publicList<CodeInfo>GetCodeInfosByGroupName(stringcodeGroupName)

修改的地方：

a、使用泛型的实体对象列表替换掉DataSet；

b、对于类型很明显的对象，不必加上类型前缀(此处sDefGrp用s代表string类型)，以减少语法噪音。我觉得只有控件名称（如：lblMessage）、“容器”类型（如：htFilters）需要类型前缀加以说明。

c、不使用缩写（def和grp都不能算是规范而通用的缩写。）

阅读全文

0 0