Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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());
    }
}
share|improve this question
2  
Will you ever want to put anything other than an int into the Stack? In that case, you should take a look at generics. In one of my previous reviews, you can see how a solid Stack implementation might look like. –  codesparkle Sep 2 '12 at 9:58

2 Answers 2

up vote 6 down vote accepted

Some small notes:

  1. According to the Code Conventions for the Java Programming Language,

    if statements always use braces {}.

    Omitting them is error-prone.

  2. 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--;
    }
    
  3. 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];
    }
    
  4. The default value of int fields are 0, therefore numElements = 0; is unnecessary in the constructor.

share|improve this answer
1  
good points; although the error message for passing too few maxElements into the constructor is wrong, should be "maxElements cannot be less than 0, was: " in accordance with the condition in the if-clause. –  codesparkle Sep 2 '12 at 9:42
1  
Point 1 can be contentious. It largely depends on the code standards you want to enforce (or is enforced at your workplace). If you look through the Java standard libraries you will find many instances of brackets not being used for if blocks. My personal preference is brackets for multiline conditionals, no brackets for single line conditionals. I break lines at 80. –  James McMahon Sep 2 '12 at 12:32
1  
Don't use IllegalStateException. Java provides the more specific EmptyStackException and StackOverflowError. –  Eva Jan 16 '13 at 20:06
1  
@Eva: EmptyStackException is a great insight, thank you! I've updated the answer. StackOverflowError is a subclass of VirtualMachineError which does not seem appropriate here. –  palacsint Jan 16 '13 at 21:12
1  
Good point. I think creating a StackOverflowException subclass of IllegalStateException is justified. –  Eva Jan 16 '13 at 21:26

Why split top and pop?

pop could easily return an int and adjust the number of elements.

In cases were the consumer wanted to pop without the int, they would just not assign the return of pop to a variable. And I can't think of a valid case for using top without using pop.

share|improve this answer
    
another good reason for pop to return the popped element is consistency: java.util.Stack and every other implementation I can think of works this way. However, a peek method should remain to check the top element without removing it (there are lots of valid use cases for examining the next item without removing it). Edit: No. It does not print 4 twice because two separate stacks are involved. Run the code, please. –  codesparkle Sep 2 '12 at 12:48
    
That's true, but it's a bit of a rabbit hole to go down. Because once you start asking if your custom stack class should use the Java stack API, I think your next question should be, do I even need a custom stack in the first place? And maybe you do for a primitive type, but then again someone has probably already done it for you. See trove4j.sourceforge.net/javadocs/gnu/trove/stack/TIntStack.html. –  James McMahon Sep 2 '12 at 12:57
    
@codesparkle, You are correct, I misread the test code. –  James McMahon Sep 2 '12 at 12:58

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.