[Mono-devel-list] Re: [Mono-patches] mcs/class/corlib/System.Collections.Generic Collection.cs,NONE,1.1

[Ben Maurer]

Hi,

Here are a few comments:

On Wed, 2004-09-01 at 19:41, Carlos Alberto Cortes  wrote:
> 		private const int defaultLength = 16;
Can you double check that this is the case? I think in List <T> it is now 4.

Style: s/private //
> 		private T [] contents;
> 		private readonly Type innertype;
This is not needed at all. You can just say typeof (T)
> 		private int capacity;
capacity == contents.Length. This is not needed.
> 		private int count;
> 		private int modcount;

> 
> 		public Collection (List <T> list)
> 		{
> 			if (list == null)
> 				throw new ArgumentNullException ("list");
> 
> 			contents = new T [list.Count * 2];
Check that the capacity is set to list.Count * 2. It is probably not.
> 			list.CopyTo (contents, 0);
> 			count = list.Count;
> 
> 			capacity = contents.Length;
> 			innertype = typeof (T);
> 				
> 		}
> 

Just make this `return IndexOf (item) > 0'.


> 
> 		IEnumerator IEnumerable.GetEnumerator ()
> 		{
> 			return new Enumerator (this);
> 		}

> 
> 		public int IndexOf (T item)
> 		{
> 			for (int i = 0; i < count; i++)
> 				if (contents [i].Equals (item))
> 					return i;
What if contents [i] == null?

I think what may happen is this:

if (item == null)
	for (int i = 0; i < count; i ++)
		if (contents [i] == null)
			return i;
else
	for (int i = 0; i < count; i ++)
		if (item.contents [i])
			return i;

> 			return -1;
> 		}

> 		protected virtual void InsertItem (int index, T item)
> 		{
Does this method check its arguments?
> 			modcount++;
> 			//
> 			// Shift them by 1
> 			//
> 			int rest = count - index;
> 			if (rest > 0)
> 				Array.Copy (contents, index, contents, index + 1, rest);
> 
> 			contents [index] = item;
> 			count++;
> 		}

> 		protected virtual void RemoveItem (int index)
> 		{
Does this method check its arguments?

Does it resize the array?
> 			modcount++;
> 			
> 			int rest = count - index - 1;
> 			if (rest > 0)
> 				Array.Copy (contents, index + 1, contents, index, rest);
> 			//
> 			// Clear the last element
> 			//
> 			Array.Clear (contents, --count, 1);
> 		}
Don't use Array.Clear for one item. You should be able to set it to the
default value (I forget the syntax  for that)

> 
> 		protected virtual void SetItem (int index, T item)
> 		{
Check arguments?
> 			modcount++;
> 			contents [index] = item;
> 		}

> 		private void CheckType (object value)
> 		{
> 			if (!innertype.IsInstanceOfType (value)) {
just say
if (!(value is T))
> 				string valtype = value.GetType ().ToString ();
> 				throw new ArgumentException (
> 						String.Format ("A value of type {0} can't be used in this generic collection.", valtype),
> 						"value");
> 			}
> 		}
> 
> 		private void Resize (int newCapacity)
> 		{
> 			T [] tmp = new T [newCapacity];
> 			Array.Copy (contents, tmp, count);
use Array.Resize here.
> 			contents = tmp;
> 			capacity = newCapacity;
> 		}
> 		
> 		//
> 		// Will we use this iterator until iterators support is done?
> 		//
> 		private struct Enumerator : IEnumerator <T>, IEnumerator 
Is this struct private in the .NET api? 


-- Ben