Implementing an array backed list

Question

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:

public class DynamicArray<T> : IEnumerable<T>, ICollection<T>
{    
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<T> GetEnumerator()
{
    if (_items == null)
        return Enumerable.Empty<T>().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();
}
}

Answer 1

You have inconsistent spacing around your operators:

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<bool> 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.