HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Java and stacks: correct implementation?

Submitted by: @import:stackexchange-codereview··
0
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?

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.

-
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.