HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Implementation of a generic List

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationlistgeneric

Problem

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: IArrayList, 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 listArray.Count())
{
throw new ArgumentOutOfRangeException();
}
if (index == 0 && arrayElementCounter == 1)
{
IfElementToBeRemovedIsHeadAndTheOnlyElement();
return;
}
else if (index == 0 && arrayElementCounter > 1)

Solution

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

Code Snippets

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;
    }
}
if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
    return;
}
else if (index == 0 && arrayElementCounter > 1)
{
    IfElementToBeRemovedIsHeadButNotTheOnlyElement(newArrayForCopying);
    return;
}
if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
}
else if (index == 0 && arrayElementCounter > 1)
// ...
if (index == 0 && arrayElementCounter == 1)
{
    IfElementToBeRemovedIsHeadAndTheOnlyElement();
    return;
}
if (index == 0 && arrayElementCounter > 1)
// ...

Context

StackExchange Code Review Q#144360, answer score: 8

Revisions (0)

No revisions yet.