ArrayIndexOutOfBoundsException in my own stack class

[akshat khandelwal]

Stack class created by me is given below

import java.util.*;
class MyStack
{
	private int c=0;//counter
	private Vector v;
	
	public MyStack()//Constructor
	{
		v=new Vector();
	}
	
	void push(Object o)
	{
		v.add(o);
		c++;
	}
	Object pop()
	{
		try
		{
			int s=v.size();
			if(s==0)
			{
				MyStackEmpty mse=new MyStackEmpty("UnderFlowException");
				throw mse;
			}
		}
		catch(MyStackEmpty e1)
		{
			System.out.print(e1.getMessage());
		}
		
		Object o=v.get(c-1);
		v.remove(c-1);
		c--;
		return o;
	}
	Object peek()
	{
		try
		{
			int s=v.size();
			if(s==0)
			{
				MyStackEmpty mse=new MyStackEmpty("UnderFlowException");
				throw mse;
			}
		}
		catch(MyStackEmpty e1)
		{
			System.out.print(e1.getMessage());
		}
		Object o=v.get(c-1);
		return o;
	}
	int search(Object o)
	{
		try
		{
			int s=v.size();
			if(s==0)
			{
				MyStackEmpty mse=new MyStackEmpty("UnderFlowException");
				throw mse;
			}
		}
		catch(MyStackEmpty e1)
		{
			System.out.print(e1.getMessage());
		}
		int p=v.indexOf(o);
		
		if(p==-1)
			return p;
		else
			return(v.size()-p);
	}
	boolean empty()
	{
		int s=v.size();
		if(s==0)
			return true;
		else
			return false;
	}
	
}

And, its MyStackEmpty class is :

class MyStackEmpty extends Exception
{
	public MyStackEmpty(String str)
	{
		super(str);
	}
}

But its peek() and pop() are throwing ArrayIndexOutOfBoundsException .
Please help

[Jesper de Jong]

That is because inside your pop() and peek() methods, you throw a MyStackEmpty exception, but you also immediately catch the exception again. The code then continues after the catch block, and you get an ArrayIndexOutOfBoundsException because you're trying to access an element of the vector with an invalid index.

Don't catch the MyStackEmpty exception inside the pop() and peek() methods. Let the exception be thrown out of the methods, so that the user of your stack class has a chance to deal with the exception. So, remove the try { ... } catch (MyStackEmpty e) { ... } from these methods.

[Vishakha Lele]

Your code is all correct. Only the issue is in what sequence did you call peek() and pop() methods after pushing the object into the stack? Because, it gives error of you first pop the element and then peek. But it runs properly if you first peek the element and then pop. The reason behind is, peek() method only peeks the object and return it. But pop actually removes the object from data structure as per your code.

[Campbell Ritchie]

I personally would do it completely different. I think the original Stack class was designed wrongly (Joshua Bloch says so, so that must be correct ‍), and I am not at all convinced that moving Stack functions to ArrayDeque made things much better. I think a Stack ISN'T A Deque and a Deque ISN'T A Stack.
I think you need a Stack interface, and then you can consider implementations. A Stack interface would have four methods for pushing popping peeking and seeing whether the stack is empty. I think you can create implementations based on a (singly‑)linked list, or an array.
Is searching a normal function of a Stack? I haven't see Stacks with a search method before.
The empty() method shou‍ld be called isEmpty() and you shou‍ld simplify it greatly. I shall give you a hint by referring you to the old Sun style guide.
Your MyStackEmpty class shou‍d have a name ending ...Exception.

[Liutauras Vilda]

1. Why the class isn't public? Methods too?

2. Campbell gave you hint, which referring to "Sun's style guide". Shoot another rabbit and learn how to format code properly.

3. There some constructs in your code, which look like:

if (A == 0)
    return true;
else
    return false;

It is quite dangerous to miss out braces, but if you don't like to use them, you can simplify all construct and be happy not using braces:

return A == 0;

4. You have some constants: 0 and -1; Why not to define them somewhere at the beginning of the class and use them, so the code would be more professional?

5. You have two comments. Both are inconsistent (one starts with a lower case, another - upper case) and useless. First comment can be deleted and variable named more clearly. Maybe just "counter"?

6. Don't import everything from util package. Import classes you are going to use. That can give to somebody and instant clue how you have implemented your stack. Use wildcard imports if you using quite a few classes from a particular package.

[Stephan van Hulst]

I'm going to disagree with you on a few counts Liutauras.

Liutauras Vilda wrote:1. Why the class isn't public? Methods too?

Actually I think this is great style, and the OP should keep it up. Only make things public when they really have to be. This should be a habit for everybody.

4. You have some constants: 0 and -1; Why not to define them somewhere at the beginning of the class and use them, so the code would be more professional?

I don't think these will be necessary. The instances where 0 is used should be replaced with a call to an isEmpty() method. Using a constant for 0 inside this method adds an extra layer of indirection that is just confusing. The same goes for 1 if it's just used as an increment or a decrement. Replacing it with a constant such as INCREMENT, STEP, or even worse, ONE is just silly. 0 and 1 are such fundamental numbers, it's usually clear from the context what they mean.

6. Don't import everything from util package. Import classes you are going to use. That can give to somebody and instant clue how you have implemented your stack. Use wildcard imports if you using quite a few classes from a particular package.

This advise holds for packages that are not widely used. If you use wildcard import for packages such as java.util, java.io, etc. it actually becomes easier to see what classes are being imported that are NOT commonly used.

[Campbell Ritchie]

Maybe if Liutauras gave the class a package name, it would require public access to most methods.
The way I would write it differently (did I really say different earlier?) the class would not require any imports at all. I don't think I would write a stack class with a List field when I can use an array or similar directly.
I have seem problems with on‑demand imports. When I was doing my MSc, one chap complained he couldn't get Timer to compile. I couldn't understand the problem at first because I had used individual imports, but he had used import java.util.*: import javax.swing.*; and there were two Timer classes. Somebody on this website once said that you can break on‑demand imports by upgrading your implementations. Let's imagine somebody had written an Objects class under Java6: import java.util.*: import uk.co.critchie.resources.Objects; All is well until Java7 comes along and they get errors (or maybe even runtime errors) about this class. If they weren't on the ball, they would have no end of difficulty working out what broke the code.
The different (correct grammar this time ‍) way I would write Stacks wouldn't require magic numbers at all: I would use the ++ and -- operators throughout except in this method:-

/**
 * ...
 */
@Override
public boolean isEmpty()
{
   return depth == 0;
}

Note that those of us who have written Forth call the number of elements on a stack its depth. Also note that the old style guide permits 0 1 and −1 as literals in code.

Those of us who have written Forth recognise four basic operations on a stack: push pop peek isEmpty. Push and isEmpty don't require any number of elements on the stack beforehand, but pop and peek require there be at least one element on the stack already. But we also recognise advanced operations, which require different minimum depths:
swap (2)
drop (1)
drop2 (2)
nip (2)
duplicate (1)
duplicate2 (2)
over (2)
rotate (3)
rotateBack (3)
and some people will recognise swap2, over2, and nip2, which require a minimum depth of 4. At least I think that's right. Forth uses different spellings.
Most people writing stacks in Java® won't need the advanced operations, but they can all be implemented by a combination of push pop and peek.

[Liutauras Vilda]

Stephan van Hulst wrote:Actually I think this is great style, and the OP should keep it up. Only make things public when they really have to be. This should be a habit for everybody.

In this particular case I still stay with my opinion. I'd agree with you if we were to talk about some ordinary classes. Such project looks like a potential candidate for OP's proprietary API, hence such advice.

Stephan van Hulst wrote:I don't think these will be necessary. The instances where 0 is used should be replaced with a call to an isEmpty() method.

That's even better, agreed.

Stephan van Hulst wrote:This advise holds for packages that are not widely used. If you use wildcard import for packages such as java.util, java.io, etc. it actually becomes easier to see what classes are being imported that are NOT commonly used.

Again, I'd re-asses my advice if we were to talk about some other situation. In this particular case we have very limited amount of classes which are used outside the lang package (in fact Vector only). If I were to chose, I'd go with explicit import. I have a habit to look at import statements once I open a class. But as you pointed out, opinions may differ.

[Stephan van Hulst]

Liutauras Vilda wrote:Such project looks like a potential candidate for OP's proprietary API, hence such advice.

Advice becomes necessity once the classes actually get used outside of their package. Make them public at that point, and no sooner.

[Campbell Ritchie]

Actually, I would have to use a little fiddle to avoid − 1 in the peek() method, creating a topElement field.

/**
 *...
 */
@Override
public final E peek()
{
   return topElement;
}

rather than

/**
 *...
 */
@Override
public final E peek()
{
   return myEArray[depth - 1]; // What's wrong with - 1 here? ;-)
}

I would probably (if awake) make all methods final, too.

[Stephan van Hulst]

I don't understand why you would even want to use an array when you have classes that handle all these trickeries for you. You may not like ArrayDeque, Campbell, but it's a fully functional tool that you can use to implement the Stack class.

final class Stack<E> {

  private final Deque<E> deque = new ArrayDeque<>();

  void push(E element) {
    deque.push(element);
  }

  // etc.

}

[Campbell Ritchie]

Stephan van Hulst wrote:I don't understand why you would even want to use an array . . .

Good idea, which I sh‍ould have got with a bit more thought. I don't not like ArrayDeque; I just think it is the wrong implementation for a stack. I would change line 1 to
[??]public[??] final class ArrayStack<E> implements Stack<E>
By Stack<E> I mean an ordinary stack without all the advanced features, but there is nothing to stop people from writing classes implementing AdvancedStack<E>.