//本文来源于一篇内部邮件,但讨论的问题具有普遍性。
程序代码有双重目的,一是供机器执行,二是供程序员阅读。而代码的质量,往往体现在第二点,也就是说,可读性好是优秀代码的主要指标。
要提高的代码的可读性,就得从以下几方面努力:
1、 清晰地表达意图
2、 一个方法只做一件事情
3、 同一个方法体内,保持相同的抽象层次
4、 不要重复自己(避免手动的复制与粘贴代码)
5、 减少“语法噪音”
6、 命名时取有意义的名字,避免不规范的缩写
提高代码的可读性的过程和方法,有一个专用名词“重构”。同时有一部名著《重构》(Martin Flower)专门讲这个问题,推荐同学们都看一下 。
我从目前的网页中抽取一个典型的方法(有时一个网页中会有几个甚至十几个结构跟它一样的方法),看看我们可以做哪些改善(以下讨论是纯技术性的,由于部分重构可能需要改变框架来支持,而框架的改变需要考虑多方面的因素,所以这里仅作理论探讨,不等同于实际应用):
public DataSet GetDefectDetails(string sDefGrp)
{
bool bResult = false;
DataSet dsData = new DataSet();
try
{
Hashtable htFilters = new Hashtable();
htFilters.Add("GROUP_NAME", sDefGrp);
bResult = fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters, ref dsData);
}
catch (Exception Ex)
{
string sErrorMessage = Helper.GetErrorDescription(ex.Message);
if (sErrorMessage == string.Empty)
{
sErrorMessage = ex.Message;
}
Master.ErrorMessage = sErrorMessage;
m_logMsg.Message = "GetDefectDetails : " + sErrorMessage;
m_logger.LogMsg(m_logMsg);
}
return dsData;
}
我们来看看,这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:
public DataSet GetDefectDetails(string sDefGrp)
{
DataSet dsData = new DataSet();
try
{
Hashtable htFilters = new Hashtable();
htFilters.Add("GROUP_NAME", sDefGrp);
fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters, ref dsData);
}
catch (Exception Ex)
{
string sErrorMessage = Helper.GetErrorDescription(ex.Message);
if (sErrorMessage == string.Empty)
{
sErrorMessage = ex.Message;
}
Master.ErrorMessage = sErrorMessage;
m_logMsg.Message = "GetDefectDetails : " + sErrorMessage;
m_logger.LogMsg(m_logMsg);
}
return dsData;
}
有同学说,这个改善太微不足道了。对的,重构的精神就是小步推进,勿因善小而不为,通过不断积累,最后就可以得到一个清晰、易维护的系统。
然后,我们的目光移向那个可疑的if语句,为什么要做这个判断?或者说,这个判断能不能放到Helper.GetErrorDescription方法内部?通过阅读Helper.GetErrorDescription方法的代码,我们发现这个判断完全可以方法内部。
在这里,造成重复代码的原因是:写共用方法的人未考虑周全,导致客户需要关心方法的条件分支情况,从外部进行判断。
通过将判断逻辑移到Helper.GetErrorDescription方法内部,代码变成15行。
public DataSet GetDefectDetails(string sDefGrp)
{
DataSet dsData = new DataSet();
try
{
Hashtable htFilters = new Hashtable();
htFilters.Add("GROUP_NAME", sDefGrp);
fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters, ref dsData);
}
catch (Exception Ex)
{
string sErrorMessage = Helper.GetErrorDescription(ex.Message);
Master.ErrorMessage = sErrorMessage;
m_logMsg.Message = "GetDefectDetails : " + sErrorMessage;
m_logger.LogMsg(m_logMsg);
}
return dsData;
}
现在再看看,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)
{
string sErrorMessage = Helper.GetErrorDescription(ex.Message);
Master.ErrorMessage = sErrorMessage;
m_logMsg.Message = "GetDefectDetails : " + sErrorMessage;
m_logger.LogMsg(m_logMsg);
}
OK,看来我们需要一个模板方法,第1个高亮的语句我们可以定义成一个委托,第2个高亮的语句是其实是方法的名称。我们可以在基类中定义:
protected delegate void MyAction();
protected void HandleException(MyAction myAction)
{
try
{
myAction();
}
catch (Exception Ex)
{
string sErrorMessage = 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);
}
}
好了,这回我们连处理异常的细节都不用关心了。由于.net 2.0可以写匿名方法,代码可以写得很简洁,新的写法只剩下8行:
public DataSet GetDefectDetails(string sDefGrp)
{
DataSet dsData = new DataSet();
HandleException(delegate()
{
Hashtable htFilters = new Hashtable();
htFilters.Add("GROUP_NAME", sDefGrp);
fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters, ref dsData);
});
return dsData;
}
好了,现在看看我们的成果:
1、 把代码从20行简化到8行(减少语法噪音)
2、 将重复的逻辑放到公用方法中(避免手动的复制粘贴代码)
3、 需要关注的地方大大减小(不用再关心bResult变量和异常处理细节,更专注于业务逻辑)
在现有框架的基础上,这已经算是一个极致,下面,让我们作一个小小的冒险,假设框架可以由我们自由定义,看看我们可以重构到什么地步。
首先,通过AOP(aspect-oriented programming)框架,我们可以让异常处理语句完全在方法体内消失,只需要在处理异常的方法上打上一个标签:
[HandleException]
public DataSet GetDefectDetails(string sDefGrp)
{
DataSet dsData = new DataSet();
Hashtable htFilters = new Hashtable();
htFilters.Add("GROUP_NAME", sDefGrp);
fisService.GetDataSet(QueryName.GET_CODE_BY_GROUP_NAME, htFilters, ref dsData);
return dsData;
}
这里体现了“声明式编程”的风格,即只要说明意图,而不需要写出处理细节。
当然以上仅为示意写法,HandleException的attribute必须另外定义,使用不同的AOP框架,写法也有不同,常用的AOP框架有Sprint.net, Castle, PostSharp等。
剩下最大的“噪音”应该就是这个Hashtable了。在这里定义Hashtable,仅仅是框架的需要,同样,QueryName也是框架的需要,如果按“正常的写法”,代码应该是:
[HandleException]
public DataSet GetDefectDetails(string sDefGrp)
{
return fisService.GetCodeByGroupName(sDefGrp);
}
OK,这回这个方法终于露出本来面目了,GetDefectDetails方法根本没有处理任何业务逻辑,之所以有20行的代码,只是因为框架本身不够完善,无法帮助程序员简化开发而已,所以…其实…这个方法…可以拿掉…
GetDefectDetails方法:哦,不!你不是来帮我改进的吗?怎么把我给杀掉了?
背景声音:不好意思,我国的改造,历来如此…
余论:
1、 问:如果方法删掉了,在哪里注明需要异常处理?
答:可以在调用GetDefectDetails方法的方法上打上HandleException的attribute,或者在配置文件中指定webForm中所有的异常使用统一的方法处理。
2、 其实我理想中的方法签名应该是:
Class CodeService…
public List<CodeInfo> GetCodeInfosByGroupName(string codeGroupName)
修改的地方:
a、使用泛型的实体对象列表替换掉DataSet;
b、对于类型很明显的对象,不必加上类型前缀(此处sDefGrp用s代表string类型),以减少语法噪音。我觉得只有控件名称(如:lblMessage)、“容器”类型(如:htFilters)需要类型前缀加以说明。
c 、不使用缩写( def 和 grp 都不能算是规范而通用的缩写。)