Proper Stack Implementation

Question

I want to know if this stack is implemented properly. Previously I was told a Linked List contains an inner class that represents a node and the outta class that represents the structure. Does that same idea apply to stacks and linked list? I am assuming not because they are so simple. Here is my implementation. Let me know if this is correct.

public class Stack{
  private int size;
  private int[] stack;

  public Stack(int height){
    stack = new int[height];
    size = 0;
  }

  public void push(int value){
    if(size == 0){

      stack[size] = value;
      size++;

    }

    else if(size == stack.length){
      System.out.println("The stack is full. The value cannot be added to the stack");
    }

    else {
    stack[size] = value;
    size++;
    }


  }

  public void pop(){

    if(size == 0){
      System.out.println("The stack is empty. Nothing can be removed from the stack");
    }
    else {
      size--;
      System.out.println(stack[size] + " was removed from the stack");

    }



  }

  public void peek(){

    if(size == 0){
      System.out.println("There is nothing to peek. The stack is empty");

    }
    else
      System.out.println(size);
      System.out.println("The value: " + stack[size -1]+ " is currently at the top of the stack");

  }

  public void isEmpty(){

    if(size == 0)
      System.out.println("The stack is empty");
    else
      System.out.println("The stack is not empty");

  }
}

Why guess? what can make it better? what is wrong with it? or are you saying okay about leaving the main method out. — jillian
– jillian, Commented Jul 26, 2016 at 10:40
For some reason I like your decision on naming the parameter for the constructor height. It is a very intuitive name for describing the maximum size of a stack. Well done :) — Thijs Riezebeek
– Thijs Riezebeek, Commented Jul 26, 2016 at 14:00

Pimgd · Accepted Answer · 2016-07-27 22:08:19Z

public void push(int value){
    if(size == 0){

        stack[size] = value;
        size++;

    }

    else if(size == stack.length){
        System.out.println("The stack is full. The value cannot be added to the stack");
    }

    else {
        stack[size] = value;
        size++;
    }


}

Let's talk about this bit of code.

First, I personally prefer 4 spaces of indentation, so I made it 4 spaces of indentation. You'll have to bear with it. (It's also a common standard, so you may have to bear with it in other places as well).

I also indented the else case, because it seems you forgot to indent those lines of code.

So. First thing I see is that you have duplication:

    if(size == 0){

        stack[size] = value;
        size++;

    }

    else {
        stack[size] = value;
        size++;
    }

And there's only one check in between. So I think you could simplify by getting rid of the 0 case and just relying on the check to keep the stack from overflowing. (Also, have you considered a stack of size 0?)

public void push(int value){
    if(size == stack.length){
        System.out.println("The stack is full. The value cannot be added to the stack");
    }

    else {
        stack[size] = value;
        size++;
    }


}

Second thing I see is the rather large amount of blank lines. There are uses for blank lines - mostly for separating sections of code that need to be in the same block, but do different things. What you have here, however, is an if-elseif-else chain (before we shortened it to an if-else chain), and you shouldn't put blank lines between them. It makes it hard to see that the else is an else and not an if. Imagine this bug-prone scenario:

if (condition){
    //code....
}
else



if (othercondition){
   //code...
}

You'd think the second if-block is on its own, but it's actually the else-if of the first if statement. Don't add blank lines in an if-elseif-else chain.

public void push(int value){
    if(size == stack.length){
        System.out.println("The stack is full. The value cannot be added to the stack");
    }
    else {
        stack[size] = value;
        size++;
    }
}

Now, depending on style, you could put the else on the same line of the ending brace, like so...

public void push(int value){
    if(size == stack.length){
        System.out.println("The stack is full. The value cannot be added to the stack");
    } else {
        stack[size] = value;
        size++;
    }
}

This (in my opinion) makes it easier to see that this is an else and not an if (when quickly reading). But that's a minor style thing.

Okay, cool. Thank you for the feedback. Very useful information. — jillian
– jillian, Commented Jul 26, 2016 at 11:00

forsvarir · Accepted Answer · 2016-07-26 11:06:26Z

Stack is a black hole from other code

The first impression I get is you can't get anything back once you put it in the Stack.

peek and pop both return void, so once you've put your value in with push that's the end of it. I know your methods output to the console, but that's pretty useless to the rest of your program.

When you're writing things like data structures try to put the user interaction outside of the class you're modelling. Both for input and output. This will encourage you to define methods that actually allow you to use the data structure in the way that you would in a normal program. With that in mind, peek at least should return a value. isEmpty should return a bool. Depending on your approach, pop could also return the value popped, or it could simply remove the item (however if you go down this approach I would expect it to throw a exception if you pop from an empty stack).

I would also be expecting push to throw an exception if the stack is full. At the moment, the client has no idea that their value hasn't been added to the stack.

Stack Exchange Network

Proper Stack Implementation

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Proper Stack Implementation

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions