Your stack is just one step away from being generic
, which is great. I don't know if it's useful to you but usually, data structures are generics!
In the initialisation, I could pass -102
as a paramter to your Stack
's length, which is flawed.
There's a bug in your push
method. If I create new Stack(0)
, an IndexOutOfRangeException
will be thrown when I push something, since you start with s[0]
. But in the other cases, you expand your array to make sure you don't have an exception. This might not be a bug, but you should think about this behavior. Plus, expanding your array with a 0
length wouldn't work, since 0*2=0
As @holroy pointed out in the comments (I'm adding it to my answer just because he/she didn't post an answer yet), you're going to throw an exception if you try to pop
an empty stack. To counter this, I added a check before popping!
If my Stack
contains : {1,2,3}
, I'm going to pop the value 3
, and return 2
. The expected behavior would be to return 3
since it's the poped value.
Also, instead of looping all the time to find the index to insert or pop in your stack, why don't you just keep this index as a private member of your class? You'll save useless while
loops and conditions.
There goes the final result :
public class Stack<T>: IStack<T>
{
private T[] s;
private int currentStackIndex;
public Stack(int N)
{
if(N < 0)
throw new ArgumentOutOfRangeException("N");
s = new T[N];
currentStackIndex = 0;
}
public void push(T x)
{
if (currentStackIndex + 1 >= s.Length)
{
//Notice the + 1, to counter the 0 length problem
Array.Resize(ref s, (s.Length + 1)*2);
}
s[currentStackIndex++] = x;
}
public T pop()
{
if(currentStackIndex == 0)
throw new InvalidOperationException("The stack is empty");
T value = s[--currentStackIndex];
s[currentStackIndex] = default(T);
return value;
}
}
I didn't talk about the naming since it's already covered.
...
on all the summary pages.) – EBrown yesterdaySystem.Collections.Generic
– Nathan Cooper 9 hours ago