4
\$\begingroup\$

The code that I wrote is as follows:

public  class Astack implements StackInterface {
private int sz;
private Object[] a;
private int topPosition = -1;
public Astack(int sz){
   a = new Object[sz];
   this.sz = sz;
}
public int size(){
return topPosition+1;
}
public boolean isEmpty(){
  if(topPosition == -1){
  return true;
}
  return false;
}
public void push(Object element) throws StackFullException{
  if(topPosition+1 == sz){
    throw new StackFullException("Stack Full.");
  }
  a[++topPosition] = element;
}
public Object top() throws StackEmptyException{
  if(topPosition == -1){
    throw new StackEmptyException("Stack already Empty");
  }
return a[topPosition];
}
public Object pop() throws StackEmptyException{
  if(topPosition == -1){
    throw new StackEmptyException("Stack already Empty");
  }
  return a[topPosition--];
}
}

The StackInterface that I wrote:

public interface StackInterface{
    public int size();
    public boolean isEmpty();
    public Object top() throws StackEmptyException;
    public void push(Object element) throws StackFullException;
    public Object pop() throws StackEmptyException;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Is this a homework problem? Is StackInterface given to you, or did you write it too? Could you also include it in the question? \$\endgroup\$ – 200_success Aug 28 '16 at 14:04
  • \$\begingroup\$ @200_success.I - is not a homework problem. I also wrote StackInterface. I just wanted to use stack the traditional way than using Stack java class. I have added the interface that i wrote in the question above. \$\endgroup\$ – Abhay_maniyar Aug 28 '16 at 16:17
5
\$\begingroup\$
private int sz;

Rather than call this sz, consider calling it capacity. Writing out whatever name is easier to read, and capacity makes it clearer that it is potential size not current size. But you don't actually need to store it. It's already stored as a.length.

public Astack(int capacity){
    a = new Object[capacity];
}

See, more readable.

public int size(){
return topPosition+1;
}

It's more customary to track the number of items in the stack, not the top position.

public int size() {
    return size;
}

Put those together.

public class Astack implements StackInterface {

    private Object[] a;
    private int count = 0;

    public Astack(int capacity){
        a = new Object[capacity];
    }

    public int size(){
        return count;
    }

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

    public void push(Object element) throws StackFullException {
        if (count == a.length) {
          throw new StackFullException("Stack Full.");
        }

        a[count] = element;
        count++;
    }

    public Object top() throws StackEmptyException {
        if (isEmpty()) {
            throw new StackEmptyException("Stack already Empty");
        }

        return a[count - 1];
    }

    public Object pop() throws StackEmptyException {
        if (isEmpty()) {
            throw new StackEmptyException("Stack already Empty");
        }

        --count;
        return a[count];
    }

}

Now each line only does one thing. We don't update count and return something else on the same line.

We don't have to do extra math to turn topPosition into what we really want.

    private Object[] a;
    private int count = 0;

    public Astack(int capacity){
        a = new Object[capacity];
    }

In the future, you may want to replace these lines with just

    private List<T> stack = new ArrayList<>();

Then the default constructor works, so you don't need to define one. And we don't need to maintain a count, as stack.size() will return the current count.

We can get rid of the StackFullException, as the ArrayList will automatically resize as necessary.

And we can stop casting to and from Object, as generics don't require that.

\$\endgroup\$
  • \$\begingroup\$ I think top should return a[count - 1]. \$\endgroup\$ – Clashsoft Aug 28 '16 at 14:58
7
\$\begingroup\$

There isn't really much to optimize here, but there are definitely improvements you can make regarding usability and coding style.

  • use generics instead of Object, otherwise the stack is difficult to use in practice.
  • in practice, you would probably want a stack to increase its size automatically instead of throwing an exception.
  • your indentation is not consistent, making your code difficult to read.
  • you need more vertical whitespace, for example before a new method
  • your variable names could use improvement. What's a sz? What's an a? What's an Astack?
\$\endgroup\$
  • \$\begingroup\$ I do intend my code properly but when i paste it here it just gets distorted which is very irritating. Apart from that you are right and +1 for the tips :) \$\endgroup\$ – Abhay_maniyar Aug 28 '16 at 12:01
  • \$\begingroup\$ Also, the use of checked exceptions makes this very unusable at the call site because you need try catch everywhere. \$\endgroup\$ – Clashsoft Aug 28 '16 at 13:33
  • \$\begingroup\$ @Clashsoft definitely. But I wasn't sure if OP maybe extended eg RuntimeException, making the exceptions unchecked (which would seem like a good option here). \$\endgroup\$ – tim Aug 28 '16 at 13:47
  • \$\begingroup\$ @Clashsoft - That's correct. I am using couple of try catch blocks here. \$\endgroup\$ – Abhay_maniyar Aug 28 '16 at 16:19
4
\$\begingroup\$

You don't need an if statment just this:

public boolean isEmpty(){
  //Returns a boolean value
  return topPosition==-1;
}

When comparing 2 variables you get a booleanresult

\$\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.