current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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;
}
share|improve this question
3  
Is this a homework problem? Is StackInterface given to you, or did you write it too? Could you also include it in the question? – 200_success♦ yesterday
    
@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. – Abhay_maniyar yesterday
up vote 5 down vote accepted 
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.

share|improve this answer
    
I think top should return a[count - 1]. – Clashsoft yesterday
    
@mdfst13 - Thoroughly explained. +1 – Abhay_maniyar yesterday
up vote 7 down vote

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?
share|improve this answer
    
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 :) – Abhay_maniyar yesterday
    
Also, the use of checked exceptions makes this very unusable at the call site because you need try catch everywhere. – Clashsoft yesterday
    
@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). – tim yesterday
    
@Clashsoft - That's correct. I am using couple of try catch blocks here. – Abhay_maniyar yesterday
up vote 4 down vote

You don't need an if statment just this:

public boolean isEmpty(){
  //Returns a boolean value
  return topPosition==-1;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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