patternjavaMinor
Java and stacks: correct implementation?
Viewed 0 times
javastackscorrectandimplementation
Problem
I created this very simple stack concept's implementation.
Could you tell me if it is correct and clean? Do you see any bad coding habits?
You can use it with the following code:
Could you tell me if it is correct and clean? Do you see any bad coding habits?
public class MyStack
{
private static final int MAXELEMENTS = 10;
private int[] elements;
private int numElements;
public MyStack()
{
numElements = 0;
elements = new int[MAXELEMENTS];
}
public boolean isEmpty()
{
return (numElements == 0);
}
public boolean isFull()
{
return (numElements == MAXELEMENTS);
}
public void push(int e)
{
if (!isFull())
elements[numElements++] = e;
}
public int top()
{
if (!isEmpty())
return elements[numElements - 1];
else
return -1;
}
public void pop()
{
if (!isEmpty())
numElements--;
}
}You can use it with the following code:
class MyStackTestDrive
{
public static void main(String[] args)
{
MyStack s1 = new MyStack();
MyStack s2 = new MyStack();
s1.push(2);
s2.push(4);
System.out.println(s1.top());
System.out.println(s2.top());
}
}Solution
Some small notes:
-
According to the Code Conventions for the Java Programming Language,
if statements always use braces {}.
Omitting them is error-prone.
-
-
I'd make the maximal number of elements configurable via the constructor:
-
The default value of
-
According to the Code Conventions for the Java Programming Language,
if statements always use braces {}.
Omitting them is error-prone.
-
top, push and pop methods handle invalid client calls too generously. You shouldn't do that. Crash early. Does it make sense to pop from an empty stack? It is rather a bug in the client code. Instead of returning -1 or swallowing the client error throw an IllegalStateException or an EmptyStackException. See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies. You could use guard clauses too: public void push(final int e) {
if (isFull()) {
throw new IllegalStateException();
}
elements[numElements++] = e;
}
public int top() {
if (isEmpty()) {
throw new EmptyStackException();
}
return elements[numElements - 1];
}
public void pop() {
if (isEmpty()) {
throw new EmptyStackException();
}
numElements--;
}-
I'd make the maximal number of elements configurable via the constructor:
private static final int DEFAULT_MAX_ELEMENTS = 10;
private final int maxElements;
private final int[] elements;
private int numElements;
public MyStack() {
this(DEFAULT_MAX_ELEMENTS);
}
public MyStack(final int maxElements) {
if (maxElements < 1) {
throw new IllegalArgumentException("maxElements cannot be less than 1, was: " + maxElements);
}
this.maxElements = maxElements;
elements = new int[maxElements];
}-
The default value of
int fields are 0, therefore numElements = 0; is unnecessary in the constructor.Code Snippets
public void push(final int e) {
if (isFull()) {
throw new IllegalStateException();
}
elements[numElements++] = e;
}
public int top() {
if (isEmpty()) {
throw new EmptyStackException();
}
return elements[numElements - 1];
}
public void pop() {
if (isEmpty()) {
throw new EmptyStackException();
}
numElements--;
}private static final int DEFAULT_MAX_ELEMENTS = 10;
private final int maxElements;
private final int[] elements;
private int numElements;
public MyStack() {
this(DEFAULT_MAX_ELEMENTS);
}
public MyStack(final int maxElements) {
if (maxElements < 1) {
throw new IllegalArgumentException("maxElements cannot be less than 1, was: " + maxElements);
}
this.maxElements = maxElements;
elements = new int[maxElements];
}Context
StackExchange Code Review Q#15262, answer score: 6
Revisions (0)
No revisions yet.