提高代码的可读性——从一个例子说起

//本文来源于一篇内部邮件，但讨论的问题具有普遍性。

程序代码有双重目的，一是供机器执行，二是供程序员阅读。而代码的质量，往往体现在第二点，也就是说，可读性好是优秀代码的主要指标。

要提高的代码的可读性，就得从以下几方面努力：

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都不能算是规范而通用的缩写。）