Is it needed to “beautify” this Stack implementation?

Question

I'm just learning OOP, and I have an assignment to create a Stack. I came up with the following. Is it "beauty" enough, or to "schooly". Where can I evolve?

This is the class:

class Stack
{        
    private int _length;        
    private List<int> _members = new List<int>();

    public Stack()
    {
        _length = 10;
    }

    public Stack(int length)
    {
        _length = length;
    }

    public List<int> Members
    {
        get { return _members; }
    }

    public int Length
    {
        get { return _members.Count; }

    }

    public bool IsEmpty
    {
        get { return (_members.Count == 0); }
    }

    public void Push(int member)
    {
        if (_members.Count < _length) _members.Add(member);
        else throw new Exception("The Stack is full.");
    }

    public int Pop()
    {
        if (!IsEmpty)
        {
            int last_one = _members[_members.Count - 1];
            _members.RemoveAt(_members.Count - 1);
            return last_one; 
        }
        else throw new Exception("The Stack is empty.");
    }

}

And this is my Form:

public partial class Form1 : Form
{
    Stack verem = new Stack(20);
    Random rnd = new Random();

    public Form1()
    {
        InitializeComponent();
    }

    private void refreshStack()
    {
        lbStack.Items.Clear();
        foreach (int member in verem.Members)
        {
            lbStack.Items.Insert(0, member);
        }
    }
    private void btnIsEmpty_Click(object sender, EventArgs e)
    {
        MessageBox.Show(verem.IsEmpty.ToString());
    }

    private void btnLength_Click(object sender, EventArgs e)
    {
        MessageBox.Show(verem.Length.ToString());
    }

    private void btnPush_Click(object sender, EventArgs e)
    {            
        verem.Push((int)rnd.Next());
        refreshStack();
    }

    private void btnPop_Click(object sender, EventArgs e)
    {
        verem.Pop();
        refreshStack();
    }

}

Answer 1

class Stack

Stack (or any other collection) is a prime example of type that should be generic.

private int _length;

I think length is not a good name for this. You should rename it to something like _maxLength, or even better, _maxCount (see below).

Also, I'm not completely sure if this feature is necessary, but it could make sense (for example System.Collections.Generic.Stack<T> doesn't have anything like this, but System.Collections.Concurrent.BlockingCollection<T> does).

_length = 10;

I don't think 10 is a reasonable default here. It would certainly be unexpected to me. A much better default would be infinity. Or maybe don't have a default at all.

public List<int> Members

Unless absolutely necessary, you should never expose private implementation details like this. Using this member, anyone can do anything to the underlying collection (including adding items beyond the allowed maximum number).

If you want to be able to iterate over the items in the collection, you should implement IEnumerable<T>.

public int Length

The name Length makes some sense for arrays, but not so much for other collections. For consistency with other collections, the best name would be Count.

return (_members.Count == 0);

The parentheses are not necessary here.

if (_members.Count < _length) _members.Add(member);
else throw new Exception("The Stack is full.");

You probably shouldn't write the body of if and else on the same line like this. Putting them on a line of their own will make your code clearer to read.

throw new Exception("The Stack is full.")

You shouldn't throw Exception directly, you should create your own type that inherits from Exception and throw that. Alternatively, you could use an existing exception type, like InvalidOperationException.

if (!IsEmpty)
{
    …
}
else throw new Exception("The Stack is empty.");

You could simplify this by reversing the condition and putting the exception first. This way, the main code will be less indented.

public partial class Form1 : Form

You should name your forms with more descriptive names, even if it's something as simple as MainForm.

lbStack

This form of Hungarian notation is usually frowned on. If you want to indicate that the variable is a list box, you can name is something like stackListBox.

(int)rnd.Next()

You don't need a cast here, Next() already returns an int.

Answer 2

Some thoughts:

• Indent your code or type Ctrl-E-D in Visual Studio, this will reformat your code automatically. It makes it more readable.

• Rename your _length variable and your length parameter to _maxCapacity and maxCapacity. This makes its meaning clearer.

• A List<T> grows automatically, so either drop your maximum capacity constraint or use an array instead, because arrays have a fixed length once they have been created. Since you can get the array length with a.Length, drop the _length (or _maxCapacity) field and instead use a _count field in order to know how many items are in the stack (in case you chose the second variant).

• By adding a generic type parameter T to the class and replacing every occurrence of int representing a stack item by T you would get a generic stack for free.

  public class Stack<T>
  {
      private List<T> _members = new List<T>();
      // OR
      private T[] _members = new T[_maxCapacity];
      ...
  

  Then you can create stacks like this:

  var stringStack = new Stack<string>(10);
  stringStack.Push("Hello");
  var doubleStack = new Stack<double>(10);
  

• Let the Members property return an IEnumerable<int> (or IEnumerable<T>) or let the Stack class implement IEnumerable<T> directly (if you don't know how to do that, changing the return type of the Members property is good enough to begin with). Exposing _members through a list allows manipulations of it from outside. IEnumerables can only be read. If you need to return a list or an array then return a copy of _members, not the original.