Implementation of a generic List

Question

I have only been self teaching myself programming on and off for about 5 months. The purpose of me writing this class is to incorporate the knowledge that I have gathered since I started.

using System;
using System.Collections;
using System.Linq;

namespace GenericDataStructures
{
   public sealed class ArrayList<T>: IArrayList<T>, IEnumerable
   {
    private const int DEFAULT_SIZE = 8;
    private T[] listArray;
    private int arrayElementCounter = 0;
    private T[] newArrayForCopying = null;

    public ArrayList()
    {
        listArray = new T[DEFAULT_SIZE];
    }

    public int Count{get{return this.arrayElementCounter;}}

    public void Add(T element)
    {
        growIfArrayIsFull();
        this.listArray[arrayElementCounter] = element;
        this.arrayElementCounter++;
    }

    public void Remove(T element)
    {
        int indexOfFoundElement = IndexOf(element);
        if (indexOfFoundElement != -1)
        {
            newArrayForCopying = new T[listArray.Length];
            // 3 cases to handle
            // Case 1. only one element in the list
            if (arrayElementCounter == 1)
            {
                IfElementToBeRemovedIsHeadAndTheOnlyElement();
                return;
            }
            // Case 2. Element To be removed is head but not the only element
            else if (Equals(listArray[0], element) && arrayElementCounter > 1)
            {
                IfElementToBeRemovedIsHeadButNotTheOnlyElement(newArrayForCopying);
                return;
            }
            // Case 3. Element to be removes is not the head and not the only one in the list
            else if(indexOfFoundElement > 0 && indexOfFoundElement < arrayElementCounter) // The element To be removed is not the tail
            {
                IfElementToBeRemoveIsNotHeadAndNotIsNotTail(newArrayForCopying, indexOfFoundElement);
                return;
            }
        }

        return;
    }

    public void RemoveAt(int index)
    {
        newArrayForCopying = new T[listArray.Length];
        if (index < 0 || index > listArray.Count())
        {
            throw new ArgumentOutOfRangeException();
        }
        if (index == 0 && arrayElementCounter == 1)
        {
            IfElementToBeRemovedIsHeadAndTheOnlyElement();
            return;
        }
        else if (index == 0 && arrayElementCounter > 1)
        {
            IfElementToBeRemovedIsHeadButNotTheOnlyElement(newArrayForCopying);
            return;
        }
        else if (index > 0 && index < arrayElementCounter)
        {
            IfElementToBeRemoveIsNotHeadAndNotIsNotTail(newArrayForCopying, index);
            return;
        }
        else
        {
            IfElementIsTail(newArrayForCopying);
            return;
        }

    }

    public void Clear()
    {
        listArray = new T[DEFAULT_SIZE];
    }

    public bool Contains(T element)
    {
        for (int i = 0; i <= arrayElementCounter; i++)
        {
            if (this.listArray.Equals(element))
            {
                return true;
            }
        }
        return false;
    }

    private void growIfArrayIsFull()
    {
        T[] newListArray;
        if (this.arrayElementCounter == this.listArray.Length)
        {
            newListArray = new T[this.listArray.Length * 2];
            Array.Copy(listArray, newListArray, arrayElementCounter);
            listArray = newListArray;
        }
        else
        {
            return;
        }
    }

    public IEnumerator GetEnumerator()
    {
        // This is not the most elegant solution. 
        // Will Change it when I understand more about 
        // what the this method is supposed to return
        return listArray.Take(arrayElementCounter).GetEnumerator();
    }

    public int IndexOf(T element)
    {
        // returns index of first instance of an element if present in the list
        // else returns an index of -1
        int index = 0;
        while (index != arrayElementCounter)
        {
            if (Equals(element, listArray[index]))
            {
                return index;
            }
            index++;
        }
        return -1;
    }

    private void IfElementToBeRemovedIsHeadAndTheOnlyElement()
    {
        listArray = new T[DEFAULT_SIZE];
        arrayElementCounter--;
        return;
    }

    private void IfElementToBeRemovedIsHeadButNotTheOnlyElement(T[] arrayForCopying)
    {
        Array.Copy(listArray, 1, arrayForCopying, 0, this.Count -1);
        arrayElementCounter--;
        listArray = arrayForCopying;
        return;
    }

    private void IfElementToBeRemoveIsNotHeadAndNotIsNotTail(T[] arrayForCopying, int indexOfElementToRemove)
    {
        Array.Copy(listArray, 0, arrayForCopying, 0, indexOfElementToRemove + 1);
        Array.Copy(listArray, indexOfElementToRemove + 1, arrayForCopying, indexOfElementToRemove, arrayElementCounter - indexOfElementToRemove);
        listArray = arrayForCopying;
        arrayElementCounter--;
    }

    private void IfElementIsTail(T[] arrayForCopying)
    {
        Array.Copy(listArray, 0, arrayForCopying, 0, arrayElementCounter -1);
        listArray = arrayForCopying;
        arrayElementCounter--;
        return;
    }
  }
}

Answer 1

Naming

Unlike in Java, constants in C# aren't normally named with all caps and underscores (DEFAULT_SIZE).

Methods names shouldn't be lower-case (growIfArrayIsFull).

Names such as IfElementToBeRemovedIsHeadButNotTheOnlyElement are way too long, and they don't describe the method well. A method should be named for what it does, not when or why it does that. A method doesn't know who calls it and in what scenario or context it happens. That's not its responsibility. A method knows what it does itself. The calling code knows when and why to use it. That's how responsibility is dealt. A pickaxe doesn't need to know it's in a coal mine.

When a method's name starts with "if", it's already very suspicious.

Structure

newArrayForCopying is a class field, and yet methods are passing it to eachother as a parameter. This is confusing: every method has access to all class members by itself, they don't need to piggyback on other methods for that purpose.

Redundant clauses

private void growIfArrayIsFull()
{
    T[] newListArray;
    if (this.arrayElementCounter == this.listArray.Length)
    {
        newListArray = new T[this.listArray.Length * 2];
        Array.Copy(listArray, newListArray, arrayElementCounter);
        listArray = newListArray;
    }
    else
    {
        return;
    }
}

the else-return bloc doesn't serve any purpose.

The same with return calls at the end of methods (Remove, growIfArrayIsFull, IfElementToBeRemovedIsHeadAndTheOnlyElement, IfElementToBeRemovedIsHeadButNotTheOnlyElement, IfElementIsTail): redundant. These methods are exited anyway.

You don't need to use both return and else clauses. It's one too many. Eg.:

if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
    return;
}
else if (index == 0 && arrayElementCounter > 1)
{
    IfElementToBeRemovedIsHeadButNotTheOnlyElement(newArrayForCopying);
    return;
}

would make more sense as:

if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
}
else if (index == 0 && arrayElementCounter > 1)
// ...

or:

if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
    return;
}
if (index == 0 && arrayElementCounter > 1)
// ...

for reasons that I hope you can see.

listArray is always initialized to the same value (new T[DEFAULT_SIZE]). You could use a field initializer instead, and get rid of the parameterless constructor. Same effect, less lines of code.

When there is no ambiguity (like a parameter named the same as a field), and there isn't any in this code, you don't need to refer to fields by this., as in this.arrayElementCounter == this.listArray.Length. Especially since you're not consistent about it and this code sometimes does it, and sometimes not. Consistency is a highly valuable trait when it comes to code.

Other concerns

• No documentation. It would be great if you documented the class with comments, especially as it's intended to be a general-use mechanism.

• Thread safety. It doesn't provide it. Eg. if two different threads call Add at the same time, it's going to mess the state up. If thread safety isn't among your requirements, that's fine, but it's good to be explicit about it. I would put that information in documentation comments.

• Nullability. How do we want this code to handle it? Did you consider that null elements can be added, that calling code can try to remove a null element or ask what its index is, that searching for a null element can end up with a NullReferenceException when Equals is called on a null, or a null is passed to it? Food for thought.

• Remove is an operation that can fail if the element you wanted to remove wasn't present in the collection. It is customary - not only in C# - to return a value indicating whether the removal was successful or not. See https://msdn.microsoft.com/en-us/library/cd666k3e(v=vs.110).aspx

• Type safety. Why does this class support non-generic version of IEnumerable but not the generic one (IEnumerable<T>)? The latter provides type safety. Type safety is better than the lack of it.

• Unit tests. If you haven't, write unit tests for it (and submit them for code review as well!). It forces you to think better about the design, and catches bugs, like the one caught out by @RUser4512 in his answer.

Having said all that, I have to say this code is quite decent and promising for someone who have only been programming for a few months. There are slip-ups, but it shows you have good capability in structuring abstract concepts, and that's the core skill in programming.

Answer 2

Naming and conventions

You will need to impose some guidelines when it comes to naming. Per example, private and public methods are mixed, which makes it hard to read for someone willing to use your class. See per example : this stack overflow thread

Identation

If you are using (recent) versions of Visual Studio, Ctrl+k+d will automatic ident all the code for you, take care of missing spaces...

Consistency

You write :

  public void Clear()
    {
        listArray = new T[DEFAULT_SIZE];
    }

But you also use the keyword this sometimes :

public int Count{get{return this.arrayElementCounter;}}

Inside a class, you do not need to use this to access the variables of the class. Instead, a good way to keep track of the (private) fields is to name them with an underscore. Therefore, in a glance, you know what belongs and does not belong to the class.

Some bugs

I doubt this function works : you do not use the variable i and do not iterate over the elements of the vector to assess equality.

public bool Contains(T element)
{
    for (int i = 0; i <= arrayElementCounter; i++)
    {
        if (this.listArray.Equals(element))
        {
            return true;
        }
    }
    return false;
}

Memory management

I note that you use a private const int DEFAULT_SIZE = 8;. But a lot of C# libraries let the user define the default size when the container is constructed. I would encourage you to follow the same rule (it allow easy performance gains when you have an idea of the size of the container you will be working with).