2
\$\begingroup\$

My understanding is that Stack possesses LIFO property. And, based upon that I have written the following program:

// This is a sample program to demonstrate push and pop
// functionality in Stack in Java.

package collections;

public class Stack_Array {

    private static final int capacity = 3;
    int[] arr = new int[capacity];
    int top = -1;

    public void push(int pushedElement) {
        if(top >= capacity) {
            System.out.println("StackOverflow !!");
        } else {
            top++;
            arr[top] = pushedElement;
        }
    }

    public void pop() {
        if(top>=0) {
            top--;          
        } else {
            System.out.println("Stack underflow !!");
        }
    }

    public void printStack() {
        System.out.println("Elements in Stack are: ");
        for(int i = top; i >=0; i--) {
            System.out.println(arr[i]);
        }
    }

    public static void main(String[] args) {

        Stack_Array s1 = new Stack_Array();
        s1.push(23);
        s1.push(21);
        s1.push(14);

        s1.pop();
        s1.printStack();
    }

}

Please let me know if it is the correct approach. Or, is there a better way to implement the same

\$\endgroup\$
3
\$\begingroup\$

I like the consistency of your braces and indentation - well done!

The name Stack_Array should be StackArray, since as member names are camelCase, type names ought to be PascalCase, not Upper_Snake_Case.


You'll want to implement a constructor so that the capacity of the object can be parameterized, and then the calling code can look like this:

StackArray foo = new StackArray(42);
foo.push(bar);

The underlying capacity static field would need to be changed a bit though; you'll want it to be an instance field, so that each object can have its own independent value. I'd keep it final though, because it's not a value you want to be able to modify during the lifetime of an instance - beyond the constructor, nothing should be allowed to modify that value.


You'll want to throw an appropriate exception in exceptional situations:

if(top >= capacity) {
    System.out.println("StackOverflow !!");

A Stack is a data structure; other code will rely on its correct behavior, and expect a number of errors to occur in certain specific situations - for example when the caller attempts to push an element into an instance that already contains as many items as it can hold.

That said, I'd prefer a Stack implementation that simply increases its capacity as items are pushed. In any case, printing "StackOverflow !!" isn't appropriate. Making the method return a bool that the calling code can evaluate to determine whether the push/pull succeeded or not, would be a better alternative if you really want a fixed capacity.


Printing to the console isn't the job of a data structure, printStack should be replaced by a method that returns a new data structure that contains a copy of the internal array. Such a toArray method would leave it up to the calling code to decide whether they want to print the items to the console, or dump them into a log file, or whatever.


Your implementation is pretty bare-bones, you'll want to look at the java.util.Stack<E> members and possibly dig into generics. The C# System.Collections.Generics.Stack<T> class could also be an inspiration.

The pop method would be expected to return the popped item. A peek method is missing, to "peek" at the next item without popping it out of the stack.


Other observations

Between push and pop, the two have an if conditional, but one is no-op in the positive branch, while the other is no-op in the negative branch (/else block). It would be better to decide whether you test for the error condition [to throw an error and fail early] or if you test for the success condition [and throw an error in the else block] - it's usually better to fail early and reduce the indentation level:

public void foo() {
    if ({error condition}) {
        throw new SomeException();
    }
    doSomething();
}

Watch the spacing on either side of comparison operators:

for(int i = top; i >=0; i--) {

Give it the same whitespace as for the = assignment operator:

for(int i = top; i >= 0; i--) {
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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