SUMTEC -- There's a thing in my bloglet.

But it's not only one. It's many. It's the same as other things but it exactly likes nothing else...

  博客园 :: 首页 :: 新随笔 :: 联系 :: 订阅 :: 管理 ::
不知道有多少人真正仔细看过.NET Framework的CLR?如果你仔细看过的话一定和我有同样的感受:写这个类库的,有三类人:1、天才;2、普通人,不过很有经验;3、普通人,但是估计刚刚大学毕业。

比如说,大家看一下System.Security.Crytography里面的代码,写得非常的棒,非常的严谨。另外HashTable也是写得非常好的,甚至我有点怀疑这个HashTable有些地方是直接用IL写的,因为有一些地方不符合C#或者VB.NET编译器的常规输出方式。但是有的地方呢,则写得非常糟糕,虽然说功能上是达到了,可是要让我来写也不会写成这样。这种情况我是看到过的,现在不是非常记得。但是大部分的地方你会觉得中规中矩,没有什么地方需要批驳,但是也没有什么地方值得赞叹的。

今天我来给大家一个例子:Queue。这个例子你会发现有至少两个人的痕迹存在,不过这个也是绝大多数的情况。你会看到整个类从整体设计上面无可挑剔,他甚至为你准备了一个能够用于多线程上的方案——Synchronized函数返回一个包装了当前Queue对象的SynchronizedQueue对象,后者派生自Queue。甚至从命名规则的角度看,从严谨的角度看,从代码安排的风格上看,都是不错的。但是如果你仔细看每一个地方的代码,你就不由得觉得是另外一个人来编写的。或者说编写这个类的具体代码的人,肯定不是参与这个类的设计的人。因为里面可以看到一些考虑不周详的地方,例如一些重复的赋值,或者某些情况下不应该引发异常却引发了异常。前者在我后面讲到的例子里面会体现出来,而后者则可以参考Queue+QueueEnumerator::Reset里面对于版本判断的失误。

转入正题

这次要揭露的Bug在哪里呢?在Clone函数。首先我们来测试这一段代码:

            Queue q1, q2;
            q1 = new Queue();
            q1.Enqueue(1);
            q1.Enqueue(2);
            q1.Dequeue();
            q2 = (Queue) q1.Clone();
           
            foreach (object o in q2)
            {
                Debug.WriteLine(o);
            }

大家觉得Debug里面应该输出一些什么呢?应该输出的是2吧?好,大家睁大眼睛看看!什么?什么也没有看见?那就对了,这个就是一个Bug!因为实际上它输出的是null。现在让我们来看看问题代码:

public virtual object Clone()
{
Queue queue1 = new Queue(this._size);
queue1._size = this._size;
Array.Copy(this._array, 0, queue1._array, 0, this._size);

queue1._version = this._version;
return queue1;
}
问题就出在上面红底的那两行。如果你稍微看看Enqueue/Dequeue,就知道它是采用数组来模拟队列的,这种方式下面需要指出头和尾的索引值,头尾两个索引值在Queue里面分别使用_head和_tail来表示。此外,你还会知道整个Queue所使用的数组大小可能会比队列当中实际拥有的元素要大。现在你再回过头来看上面这两句,你会发现什么呢?第一,queue1._size = this._size表明作者也许认为拷贝内容的数量就应该是队列里面的实际内容的数量。第二,Array.Copy这句话表明,作者认为克隆的时候需要从数组的0下标开始,拷贝一定数量的内容,这个数量是_size——也就是队列里面实际内容的数量。那么你就不难理解,为什么上面的例子会克隆出错误的结果了:因为这个数组不可能是正好整理过的,从0开始的!

怎么修正这个代码呢?有三种方式:1、添加一定的计算和判断语句,根据实际内容所在位置进行拷贝;2、创建新的队列的大小(capacity)和当前的一样大,然后拷贝全部的内容;3、在拷贝之前对自身的队列进行整理。实际上第三种方式应该是最简单的,仅仅需要调用this.SetCapacity(_size)就可以了。而据我分析很可能作者忘了写这句话,漏掉了。

也许你会说,漏写了什么东西其实很平常,高手有时候也会犯这种错误。那么请你仔细看,那一句queue1._size = this._size 是否多余呢?上面那一句queue1 = new Queue(this._size); 为什么要传递_size参数呢?可以看出,这个作者至少不是一个熟练工。
posted on 2004-06-09 11:27  Sumtec  阅读(2250)  评论(16)    收藏  举报