字里行间有学问——从一个方法的吐槽开始,说说几个编码实践
这两天小小的维护了下一个历史遗留的基于.net 1.1开发的后台程序,加几行代码上去,这场景需要将一个ArrayList类型的返回值转换为string[]传给另一个方法,听“老人”说,已经写了个方法来干这事,OK,那就用之:
ArrayList list = GetList(); // 返回ArrayList的方法,原名不是这个 int count = list.Count; // 下文还做它用,用变量存储 // ** 就加了这么两行代码 ** string[] array = StringHelper.List2Array(list); ProcessArray(array); // 接收string[]的方法,原名不是这个 // ... 中间省略 for (int i = 0; i < count; i++) { // do somthing with list[i] }
可一运行,就“不对劲”了,为何ArrayList里5个字符串到ProcessArray接收的数组里剩4个了?怎么后面的for循环还能抛出个ArgumentOutOfRangeException来?
实在纳闷,于是几经周折,找来了List2Array方法的源码:
/// <summary> /// 列表转为字符串数组 /// </summary> public static string[] List2Array(ArrayList list) { if (list == null) return null; list.TrimToSize(); for (int i = 0; i < list.Count; ) { string str = list[i] as string; if (str == null || str.Length == 0) { list.RemoveAt(i); list.TrimToSize(); } else { i++; } } string[] strArray = null; if (list.Count > 0) { strArray = new string[list.Count]; for (int i = 0; i < list.Count; i++) { strArray[i] = list[i].ToString().Trim(); } } return strArray; }
看了代码,不到30行,却有一种五味陈杂的感觉。安抚了心中奔腾的马儿们,冷静下来,用文字总结了一下。
代码异味的来源
额外的性能开销
方法中在for循环前和循环中对于ArrayList的TrimToSize操作造成了额外的性能开销,而最终效果等同于在for循环后做一次TrimToSize。
奇怪的是,这不是我第一次看到这样用TrimToSize了,而且还是在不同的公司里,是否意味着TrimToSize方法在命名或文档上具有误导性,可能让开发者以为不断做此操作能够获得提升性能?
程序跑在一个内存充裕的环境中,而运行时传入的ArrayList大小通常不超1000,并且都是用过即丢,我并没有找到一个需要TrimToSize的理由,可以认为,这里的TrimToSize是多余的。
副作用
该方法修改了输入值,将输入的ArrayList实例中不是字符串或是空字符串的元素移除了,从而产生了副作用,导致了调用代码下文发生ArgumentOutOfRangeException,因为ArrayList的长度发生了变化。
副作用来自于对输入值的修改,这立刻让我想到了函数式编程的概念。对比此时的场景需求,我需要的正是个函数——输入值ArrayList,输出string[]——它并不依赖输入输出值外的其他状态,完全可以是个纯函数。
职能不单一,它做了太多的事情
要把这个方法方法改成纯函数,就不能修改输入值,这就纠结了,它在修改输入值上做了N件事。
要怎么处理,来逐一考虑:
将非字符串元素从ArrayList里移除——怎么处理这个问题,放在下文说明。
将空字符串从ArrayList里移除——此操作并不影响ToArray的实现,一码事是一码事,这个操作应该在别的地方做:可以是调用者所在的位置由方法调用者自行处理;若真的提供一个独立的方法(也应该是函数)来实现,它的签名可以是:
ArrayList NonEmptyElements(string[] array);
或者,应用上Linq的思想,也可以是:
IEnumerable NonEmptyElements(string[] array);
对ArrayList做TrimToSize——上文已经提到,TrimToSize就不用了,这个操作直接删掉罢。
在将复制进数组的字符串的前后空白字符移除——和前面的“将空字符串从ArrayList里移除”操作一样,应该被分离出去。
“垃圾进,垃圾出”
从代码可以看出该方法使用的“垃圾进,垃圾出”(GIGO)的做法:若list中的元素数量为0,或者仅包含null、非字符串、空字符串,则返回null。
在我们的习惯中,元素数量为0的ArrayList转为数组,也应该是一个长度为0的数组,而方法返回null,并不符合习惯,可能导致意外的发生,比如导致之后的代码访问类成员(比如Length属性)时发生空指针异常(NullReferenceException)。而对于仅包含null、非字符串、空字符串的集合返回null,则可以认为是一种不正确的“垃圾”判定,很有种越俎代庖的意味,它想当然得认为这些元素对于调用者是没有用的,也仍然犯了职能不单一的问题。
如果产生了“垃圾”,最终总得有个地方来接收和处理它们,为何不就近处理,让“垃圾”进得来、出不去,这就不得不提防御式编程了。《代码大全》中的一个章节便详细介绍了防御式编程。
对应上面的场景,我们需要“防住”什么——非字符串元素。非字符串元素如何转为字符串呢,ToString?若如此,就又越俎代庖了——替调用者决定了怎么转换。那既然这个方法里不能决定如何转换非字符串,那就只能“防住”它了,抛出个异常吧。
命名
应该给方法一个意义明确的能够说明其作用的名字,这不仅仅帮助后来的使用者更好的理解方法的作用,也让编码者获得了一个反思的机会,因为命名需要程序员花费不少脑力去思考,要不怎么说命名是程序员最头疼的事情呢。
List2Array并没有说明这个方法到底做了什么,如果确实需要这个方法,对应其功能,它的名称或许应该是:
TrimListAndRemoveNullOrEmptyStringsFromListThenTrimToArray
如此命名后,我相信方法的编写者自己就感觉到了代码异味,就可能回头调整重构,从而避免今天的这个小“悲剧”的发生。反之,使用的名称越模棱两可——比如ProcessData这种万精油——那么方法就越有可能是个大杂烩。
修改后的结果
综合上面的内容:
- 纯函数;
- 职能单一;
- 防御式编程;
我们得到修改后的方法:
public static string[] ToArray(ArrayList list) { if (list == null) return null; string[] array = new string[list.Count]; for (int i = 0; i < list.Count; i++) { // 防住非字符串元素,使用强制转换,对非字符串元素抛出InvalidCastException array[i] = (string)list[i]; } return array; }
写完后,可以发现它和ArrayList的内置ToArray方法功能是一致的,我们需要的其实只是这么一行代码:
string[] array = list.ToArray(typeof(string));
而我们发现上面自己写的ToArray方法从一开始就用不着写它。
另外,从.net2.0开始,泛型集合的引入解决了集合元素类型不明确的问题。再之后的.net3.5引入Linq后,要获得原方法的返回值就更简单了(不过没有修改原list的功能):
string[] array = list.OfType<string>() .Where(x => !string.IsNullOrWhiteSpace(x)) .Select(x => x.Trim()) .ToArray();
总结
分析List2Array方法被编写出来的原因,可能是开发者对于.net标准库还不够熟悉,也可能是开发者希望一个调一个方法就能搞定一堆事情。
标准库的方法多是经过精心设计的,已经提供了诸多基础操作,了解并灵活使用它们,避免重新造轮子。在添加一个公共方法时,问自己:我真的需要写它吗?在调用一个方法时,问自己:我真的需要用到它吗?尤其工具库的方法,被不同开发者复用的概率最大,被用得越多,就越需要在意方法是不是简洁高效,有没有什么奇怪的行为、要求使用者遵守某些奇怪的约定,需要注意的细节过多时,可能带来的就不是效率的提升而是各种诡异的bug了。
写这一个方法事小,可一行行代码,都包含了许多编程思想与最佳实践,需要字斟句酌,慎之又慎。
最后,2014年即将到来。Happy New Year!