patterncsharpMinor
Implementing an array backed list
Viewed 0 times
arraylistimplementingbacked
Problem
I post the below code as review/answer to a question in codereview.
(Custom Dynamic Array)
however, seems the answer itself having issues looking at the comments that I received.
I would appreciate your valuable review comments:
(Custom Dynamic Array)
however, seems the answer itself having issues looking at the comments that I received.
I would appreciate your valuable review comments:
public class DynamicArray : IEnumerable, ICollection
{
private T[] _items;
public DynamicArray()
{
}
public int Count
{
get
{
if (_items == null)
return 0;
return _items.Length;
}
}
public bool IsReadOnly => false;
public void Add(T item)
{
if(item==null)
throw new ArgumentException("item");
if (_items == null)
{
_items = new T[1];
}
else
{
Array.Resize(ref _items,_items.Length+1);
}
_items[_items.Length - 1] = item;
}
public void Clear()
{
_items = null;
}
public bool Contains(T item)
{
if (_items == null)
return false;
return _items.Contains(item);
}
public void CopyTo(T[] array, int arrayIndex)
{
_items.CopyTo(array, arrayIndex);
}
public IEnumerator GetEnumerator()
{
if (_items == null)
return Enumerable.Empty().GetEnumerator();
return _items.AsEnumerable().GetEnumerator();
}
public bool Remove(T item)
{
if (item == null)
return false;
if(_items==null)
return false;
var temp = _items.Where(i => !i.Equals(item));
_items = temp.ToArray();
return true;
}
IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}
}Solution
You have inconsistent spacing around your operators:
vs
I generally prefer to see the second version (as I think is fairly standard in C#).
As you're using Expression Bodied Members. I know you're using C#6. Did you know about the
Becomes
That gives you compile time checking on the name of your parameter.
You should throw the most specific exception type available. If the argument is null and that isn't allowed.
A list
You resize the array on every
Also, under the covers, an
You have a bunch of things like this:
Which could be written more simply as:
You could also use the
But it's a bit ugly because you have to do something to go from
Your
All your class is doing is keeping an array as a field and replacing it every time you modify the collection. It's not a very efficient way of doing it. As I said, you should be keeping an array with spare capacity that you resize when you need.
That's everything off the top of my head. I'll come back later when I have more time.
if(item==null)vs
if (_items == null)I generally prefer to see the second version (as I think is fairly standard in C#).
As you're using Expression Bodied Members. I know you're using C#6. Did you know about the
nameof operator? throw new ArgumentException("item");Becomes
throw new ArgumentException(nameof(item));That gives you compile time checking on the name of your parameter.
You should throw the most specific exception type available. If the argument is null and that isn't allowed.
throw new ArgumentNullException(nameof(item)).A list
DynamicArray that doesn't allow null values is really odd. There is no reason why you should disallow null. You wouldn't stop me adding 0 to a list of integers.You resize the array on every
Add. That's O(n) and unnecessary. You should be creating an array larger than your current limit and then making it bigger (by some factor) when you reach the limit.Also, under the covers, an
Array.Resize simply creates a new array and copies everything over. You have a bunch of things like this:
public bool Contains(T item)
{
if (_items == null)
return false;
return _items.Contains(item);
}Which could be written more simply as:
return _items != null && _items.Contains(item);You could also use the
?. null propagation operator: return _items?.Contains(item) == true;But it's a bit ugly because you have to do something to go from
Nullable to a bool. I achieved that with a simple == true to force an implicit conversion.Your
Remove is incorrect. It should only remove the first occurrence of the item. See msdn.All your class is doing is keeping an array as a field and replacing it every time you modify the collection. It's not a very efficient way of doing it. As I said, you should be keeping an array with spare capacity that you resize when you need.
That's everything off the top of my head. I'll come back later when I have more time.
Code Snippets
if(item==null)if (_items == null)throw new ArgumentException("item");throw new ArgumentException(nameof(item));public bool Contains(T item)
{
if (_items == null)
return false;
return _items.Contains(item);
}Context
StackExchange Code Review Q#126101, answer score: 4
Revisions (0)
No revisions yet.