2011年3月16日
11:41
这两天Code Diff,看到如下的代码,功能比较简单,技术判断两个HashObject对象是否相等:
private bool Equals(IHashObject data1, IHashObject data2)
{
bool value = true;
foreach (string tempString in data1.Keys)
{
if (!string.Equals(data1[tempString], data2[tempString]))
{
value = false;
break;
}
}
return value;
}
看到这个,凡是知道“防卫语句”重构的,都会建议修改:
private bool Equals(IHashObject data1, IHashObject data2)
{
foreach (string tempString in data.Keys)
if (!string.Equals(data[tempString], hash[tempString]))
return false;
return true;
}
这样就可以省掉一个中间变量,顺便也把不必要的“{}”删除。看起来就比较清爽了。
开发工程师提出:“尽管这样做很漂亮,不过我听说有一个设计原则,就是控制块应该是单入单出的。就是说foreach内要退出控制块应该用break,而不是return,这样的话代码可能会更好阅读”。他知道这样原则,但是并不知道它的所以然。我也不知道,当时学过这个规则,就简单的记下了。
回家后想明白了。用break可以让控制块和其他控制块独立运作,可以达到分而治之的目的。而如果跳转比较复杂,那么就需要几个块同时阅读,导致混乱的局面——也就是所谓的面条代码。不过这里的情况显然不需要遵守此规则。因为
<!--[if !supportLists]-->1. <!--[endif]-->这里的控制块只有两个,无需担心无法分而治之
<!--[if !supportLists]-->2. <!--[endif]-->这个规则主要针对乱跳的goto语句,相比之下return的问题并不大。
大家也接受了这个观点。
后记:
过了两天,我回顾了下代码,觉得这个代码其实根本不必写。因为Equals方法已经被IHashObject重载过了。就是说data1.Equals(data2) 和Equals(data1,data2)是等效的。呵呵。