patterncsharpMinor
Implementation of a generic List
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)
```
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 (
Methods names shouldn't be lower-case (
Names such as
When a method's name starts with "if", it's already very suspicious.
Structure
Redundant clauses
the else-return bloc doesn't serve any purpose.
The same with
You don't need to use both
would make more sense as:
or:
for reasons that I hope you can see.
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
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
-
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
-
-
Type safety. Why does this class support non-generic version of
-
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.
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.