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'm still new to Java and I'm trying to create a general purpose stack queue that uses an object array. I known that I've done things incorrectly, namely declaring the arrays and assigning the array lengths.

Can someone take a look and give me some feedback?

public class GeneralStack
{
    GeneralStack [] stack;  //not sure how to declare this
    private int count;
    private static final int DEFAULT_CAPACITY = 100;

//default constructor
public GeneralStack()
    {
        stack = new int[DEFAULT_CAPACITY];
        count = 0;
    }    

//alternate constructor
public GeneralStack (int maxCapacity)
    {
        stack = new int[maxCapacity];
        count = 0;
    }

//accessor getCount
public int getCount ()
    {
    return count;
    }

//accessor isEmpty
public boolean isEmpty ()
    {
    boolean isEmpty=false;
    if (count == 0);
        {
            isEmpty=true;
        }
    return isEmpty;
    }

//accessor isFull
public boolean isFull ()
    {
    boolean isFull=false;
    if (count == maxCapacity);
        {
        isFull=true;
        }
    return isFull;
    }

//mutator push
public void push (int value)
    {
    if (isFull ())
        {
        throw new IllegalArgumentException("Stack is full");
        }
     else
        { 
        stack[value]; //not sure how to assign value to the stack
        count++;
        }
    }

//mutator pop
public void pop ()
    {
         int topVal = top();
        count = count-1;
        return topVal;
    }    

//accessor top
public int topVal ()
    {
    if (isEmpty())
        {
        throw new IllegalArgumentException("Stack is empty");
        }
    else 
        {
        topVal=stack[count-1];
        }
    return topVal;
    }
}
share|improve this question

closed as off-topic by 200_success, Malachi, ChrisWue, Morwenn, Mat's Mug Dec 5 '13 at 15:21

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Your question must contain working code for us to review it here. For questions regarding specific problems encountered while coding, try Stack Overflow. After getting your code to work, you may edit this question seeking a review of your working code." – 200_success, Malachi, ChrisWue, Morwenn, Mat's Mug
If this question can be reworded to fit the rules in the help center, please edit the question.

2  
your code doesn't compile. Aren't you using an IDE such as Eclipse or NetBeans? –  codesparkle Aug 7 '12 at 9:15
    
“stack” and “queue” are two different things (actually kind of opposites). Which did you mean? Also, have you looked at the existing containers that Java provides? How does your container offset against these in terms of required functionality? –  Konrad Rudolph Aug 7 '12 at 10:05
add comment

3 Answers

There is no such thing as a stack queue — they are completely different things. You are trying to program a Stack. And you can't really call a Stack general purpose: it's a specialized collection that supports the last in, first out (LIFO) principle.

So I guess that by "general purpose" you meant able to work with objects of all types. The solution to that is called Generics and you should study them thoroughly. Basically, a generic class will accept one or more type arguments when instantiated:

Stack<Integer> stack = new Stack<Integer>();
Stack<String> anotherStack = new Stack<String>();

This means every Stack will only accept elements of one type, protecting type safety:

stack.push(5000);                // allowed
stack.push("hi");                // compiler error
anotherStack.push(5000);         // compiler error
anotherStack.push("hi");         // allowed
System.out.println(stack.pop()); // prints 5000

So bearing that in mind, let's take a look at your code. When re-implementing a class that already exists, it's a good idea to take a look at the original. As you can see, it makes use of a Vector. That's a growable array of objects — the perfect starting point for your own implementation of Stack.

So let's start off by changing this:

GeneralStack [] stack;

To this (we'll come back to the type parameter, E, later):

private Vector<E> stack;

A quick question to ask in between: Why would anyone, ever want to limit the size of a Stack? I think you misunderstood the int capacityargument on many of the Java collections. It isn't an upper limit; it's the initial size to prevent re-allocating the interally used array too often. So let's get rid of the DEFAULT_CAPACITY and let Vector handle that for us — the same thing applies to count.

The methodsisEmpty() and size() (renamed from getCount() to match the standard naming) can now delegate their work to Vector.isEmpty() and Vector.size().

And push can change to this:

public void push(E value) {
    stack.add(value);
}

Never throw an IllegalArgumentException if you are accepting no arguments. An EmptyStackException would be much more appropriate:

public E pop() {
    if (isEmpty())
        throw new EmptyStackException();
    return stack.remove(stack.size() - 1);
}

Your method topVal() should really be called peek() and follow the same principles when it comes to throwing exceptions. Note how convenient the Vector.lastElement() method comes in here:

public E peek() {
    if (isEmpty())
        throw new EmptyStackException();
    return stack.lastElement();
}

Time to finally come back to the type parameters. Change your class definition to the following:

public class Stack<E> implements Iterable<E> {

The <E> means your Stack will accept one type parameter (you can reference it in your methods, as shown above). The implementation of Iterable:

@Override
public Iterator<E> iterator() {
    return stack.iterator();
}

allows your Stack to be used in a foreach loop:

for (int element : stack){
    System.out.println(element);
}

A few more notes about your original code. Try to simplify as far as possible. For instance, this madness:

//accessor isEmpty
public boolean isEmpty ()
    {
    boolean isEmpty=false;
    if (count == 0);
        {
            isEmpty=true;
        }
    return isEmpty;
    }

should be written as:

public boolean isEmpty()
{
    return count > 0;
}

Note that I removed all your comments because they are absolutely useless noise. Try to only write comments if you have something meaningful to say that can't be expressed in the code (or that is valuable to the caller). And try to adopt the standard Java code style.


All the above suggestions would lead to something like this:

import java.util.EmptyStackException;
import java.util.Iterator;
import java.util.Vector;

public class Stack<E> implements Iterable<E> {
    private Vector<E> stack;

    public Stack() {
        stack = new Vector<E>();
    }

    public Stack(int capacity) {
        stack = new Vector<E>(capacity);
    }

    public int size() {
        return stack.size();
    }

    public boolean isEmpty() {
        return stack.isEmpty();
    }

    public void push(E value) {
        stack.add(value);
    }

    public E pop() {
        if (isEmpty())
            throw new EmptyStackException();
        return stack.remove(stack.size() - 1);
    }

    public E peek() {
        if (isEmpty())
            throw new EmptyStackException();
        return stack.lastElement();
    }

    @Override
    public Iterator<E> iterator() {
        return stack.iterator();
    }
}

Usage

Stack<Integer> stack = new Stack<Integer>();
stack.push(5);
stack.push(3);
stack.push(10);
int result = stack.pop();
System.out.println(result); // 10
for (int element : stack){
    System.out.println(element); // 5, 3
}
System.out.println(stack.peek()); // 3
System.out.println(stack.isEmpty()); // false
stack.pop();
stack.pop();
stack.pop(); // EmptyStackException
share|improve this answer
add comment

For a beginner, study

LinkedList<YourClass> lk;

The easy way : use Eclipse, and Java SDK, create a class and type the code above, type after lk.<CTRL+SAPCE> and, if you do not have the code, connect to src.zip file in your jdk directory (like C:\Program Files\Java\jdk1.7.0_05)\src.zip.

Then, if you <CTRL+CLICK> on LinkedList you have source.

JVM is quite complete OS!

Enjoy yourself.

share|improve this answer
add comment
  1. I guess it's a homework or something similar and as the OP mentioned it uses an object array. Then,

    GeneralStack [] stack;
    

    should be

    private int[] stack; // not sure how to declare this
    

    if you want to store integers in the stack, and

    stack[value]; //not sure how to assign value to the stack
    

    should be

    stack[count] = value;
    

    The pop method should have a return type, so change

    public void pop() { ... }
    

    to

    public int pop() { ... }
    

    Finally, in the isFull method you don't have maxCapacity local variable nor field, so it does not compile, but you can use stack.length instead of maxCapacity here.

  2. I'd rename topVal to peek as the Stack class calls it.

  3. The default value of int fields is 0, so count = 0 is unnecessary in the constructors.

share|improve this answer
add comment

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