patterncsharpMinor
Is it needed to "beautify" this Stack implementation?
Viewed 0 times
thisneededstackbeautifyimplementation
Problem
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:
And this is my Form:
This is the class:
class Stack
{
private int _length;
private List _members = new List();
public Stack()
{
_length = 10;
}
public Stack(int length)
{
_length = length;
}
public List 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();
}
}Solution
class StackStack (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 doesn't have anything like this, but System.Collections.Concurrent.BlockingCollection 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 MembersUnless 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.public int LengthThe 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 : FormYou should name your forms with more descriptive names, even if it's something as simple as
MainForm.lbStackThis 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.Code Snippets
class Stackprivate int _length;_length = 10;public List<int> Memberspublic int LengthContext
StackExchange Code Review Q#33329, answer score: 6
Revisions (0)
No revisions yet.