# Proper Stack Implementation

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");

}
}

• What's the scanner import for? Is this all the code? – Pimgd Jul 26 '16 at 10:35
• I left out the main method that does console testing – jillian Jul 26 '16 at 10:38
• Okay, I guess... – Pimgd Jul 26 '16 at 10:39
• Why guess? what can make it better? what is wrong with it? or are you saying okay about leaving the main method out. – jillian Jul 26 '16 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 Jul 26 '16 at 14:00

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++;
}

}


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 Jul 26 '16 at 11:00

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.