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

[Carlos Alberto Cortez]

Hi, 
notes below:

El mié, 01-09-2004 a las 22:20, Ben Maurer escribió:
> 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.

In the longhorn documentation it is not stated (at least for
Collection); however, I will try to 
test this.

> 
> 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;
> > 		}

InsertItem, RemoveItem and SetItem are methods used inside other methods
(InsertItem is used in Insert and Add, SetItem in the indexer, and
RemoveItem in Remove). 

As long as I could test, the exceptions are checked in the calling
methods (the class is designed to be extended, and thus the coder must
only add statements in the overriden methods and call the base method).

> > 		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?

Both Add and Insert check the array capacity.

> 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? 

It looks like it is a private one.

Thanks for the response.

Carlos.

> 
> 
> -- Ben
> 
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
> 
>