current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

Pretty simple class but just want to make sure I've approached it correctly.

public class DynamicArray<T> : IEnumerable<T>, ICollection<T>
    {
        private T[] _items;
        private int _size;
        private const int _growFactor = 2;

        public DynamicArray()
        {
            _items = new T[4];
        }

        public int Count
        {
            get
            {
                return _size;
            }
        }

        public bool IsReadOnly
        {
            get
            {
                return false;
            }
        }

        public void Add(T item)
        {
            if (_size + 1 > _items.Length)
            {
                var newArray = new T[_items.Length * _growFactor];
                _items.CopyTo(newArray, 0);
                _items = newArray;
            }
            _items[_size] = item;
            _size++;
        }

        public void Clear()
        {
            _items = new T[2];
            _size = 0;
        }

        public bool Contains(T item)
        {
            foreach(var value in _items)
            {
                if (value.Equals(item))
                {
                    return true;
                }
            }
            return false;
        }

        public void CopyTo(T[] array, int arrayIndex)
        {
            _items.CopyTo(array, arrayIndex);
        }

        public IEnumerator<T> GetEnumerator()
        {
            for(var i = 0; i < _size; i++)
            {
                yield return _items[i];
            }
        }

        public bool Remove(T item)
        {
            for(var i = 0; i < _items.Length; i++)
            {
                if (_items[i].Equals(item))
                {
                    _items[i] = default(T);
                    _size--;
                    return true;
                }
            }
            return false;
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return GetEnumerator();
        }
    }
share|improve this question
1  
referencesource.microsoft.com/#mscorlib/system/collections/… – zgnilec Apr 17 at 22:44
up vote -2 down vote

I re-factored your implementation to be more efficient. Such as removing unnecessary 'for-loops' and extra variables.

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 (_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();
    }
}

Please find below the Unit-Test class that I used to validate the implementation against your original intent.

[TestClass]
public class DynamicArrayTests
{

    private DynamicArray<int> _sut;


    [TestInitialize]
    public void Init()
    {
        _sut = new DynamicArray<int>();
    }

    [TestMethod]
    public void ReadOnly_ShouldReturn_False()
    {
        Assert.IsFalse(_sut.IsReadOnly);
    }

    [TestMethod]
    public void Count_ShouldReturn_InitialCount_0()
    {
        var expectedCount = 0;
        var actualCount = _sut.Count;
        Assert.AreEqual(expectedCount, actualCount);
    }

    [TestMethod]
    public void Add_ShouldIncreaseCount()
    {
        var expectedCount = 1;

        _sut.Add(4);
        var actualCount = _sut.Count;

        Assert.AreEqual(expectedCount, actualCount);

    }

    [TestMethod]
    public void Clear_ShouldClearAllElements()
    {
        var expectedCount = 0;

        _sut.Clear();
        var actualCount = _sut.Count;

        Assert.AreEqual(expectedCount, actualCount);
    }

    [TestMethod]
    public void Contains_ElementExists_True()
    {
        _sut.Add(5);
        var actual = _sut.Contains(5);

        Assert.IsTrue(actual);
    }

    [TestMethod]
    public void GetEnumerator_FromEmptyList_ReturnList()
    {
        var list = _sut.GetEnumerator();

        Assert.IsNotNull(list);
    }

    [TestMethod]
    public void GetEnumerator_ReturnExpectedValue()
    {
        _sut.Add(25);

        var list = _sut.GetEnumerator();
        list.MoveNext();

        Assert.IsTrue(list.Current.Equals(25));

    }

    [TestMethod]
    public void Remove_ShouldRemoveGivenElement_Return()
    {
        var expectedCount = 1; 

        _sut.Add(20);
        _sut.Add(30);

        var status = _sut.Remove(20);

        var actualCount = _sut.Count;

        var contains = _sut.Contains(20);

        Assert.IsTrue(status);
        Assert.AreEqual(expectedCount, actualCount);
        Assert.IsFalse(contains);

    }

}

Also note that this implementation is not thread-safe.

share|improve this answer
2  
So, this re-sizes every add, which essentially allocates a new array every time you add a new element. Isn't this somewhat inefficient compared to growing at an exponential rate? – Scott Apr 18 at 19:57
    
@Scott, good catch. Answer is Yes and No :) "Performance. Copying your array completely when resizing it is wasteful in many situations. For these cases, use List. But sometimes Array.Resize can lead to better performance. Arrays boost memory efficiency and lookup speed." ref: (dotnetperls.com/array-resize) I always prefer using BCL implementations rather using my own solutions. Further, with this implementation, yo don't have to use additional variables to maintain state of your class. Hope above link would give you more insight into Array.Resize(...) – RSF Apr 18 at 21:47
1  
This isn't a review. This is just your implementation which has numerous issues. For example, you let me put null into the list but then I can't remove it. – RobH Apr 19 at 7:23
    
Hi @RobH, I tried to show him an implementation rather than reviewing by words. sometimes examples make sense. Thanks for pointing the null acceptance in the add(...) method. Would be appreciated you could point me the other 'numerous' issues you observed in the code. – RSF Apr 19 at 10:18
    
If you ask a question with this code in it I'll happily post a full review a bit later on. – RobH Apr 19 at 10:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.