Array List issues :P (advise is welcome) I'm new to java....

Newbie & Debugging Questions
#1

Hello,

I am having problems with a method I’m working on to handle adding and removing objects from two array lists.

public class Player {
	
	public static ArrayList<String> inventory = new ArrayList<String>();
	UserInput input = new UserInput();
	
	public void start() {
		
		if (UserInput.getInput("").equalsIgnoreCase("take book")) {
			if (Room1.room1.contains("book")) {
				inventory.add("book"); 
				Room1.room1.remove("book");
				getStatus();
				Room1.getStatus();
			}
		} else if (UserInput.getInput("").equalsIgnoreCase("drop book")) {
			if (inventory.contains("book")) {
				inventory.remove("book"); 
				Room1.room1.add("book");
				getStatus();
				Room1.getStatus();
			}
		} else if (UserInput.getInput("").equalsIgnoreCase("take paper")) {
			if (Room1.room1.contains("paper")) {
				inventory.add("paper"); 
				Room1.room1.remove("paper");
				getStatus();
				Room1.getStatus();
			}
		} else if (UserInput.getInput("").equalsIgnoreCase("drop paper")) {
			if (inventory.contains("paper")) {
				inventory.remove("paper"); 
				Room1.room1.add("paper");
				getStatus();
				Room1.getStatus();
			}
		}
		reStart();
	}

It (sorta) does what I want it to do…
But for for some reason I have to type for example “take book” like four times before it actually prints the outcome.
And for each input it seems to be random as to how many times I have to type it in before it actually prints it.

Just to clarify so you can help me better…
the reStart() method just calls the start() method again.
The getStatus() methods from both the Room1 class and Player class just print their equivalent Array list.

This is the GetInput() method…

public static String getInput(String prompt) {
		BufferedReader stdin = new BufferedReader(
				new InputStreamReader(System.in));
		
		System.out.print(prompt);
		System.out.flush();
		
		try {
			return stdin.readLine();
		} catch (Exception e) {
			return "Error; " + e.getMessage();
		}
	}

If there is more info you need to help, let me know…

Also, If you have any suggestion on a better idea on how I should do this PLEASE let me know :slight_smile:
I am very new to java coding and I am learning, this is a text adventure I am working on…

Thank you!

#2

I don’t see anything wrong with the handling of “take book”. However, I see a HUGE bug: you are taking user input each time it steps through the if-else ladder. For example, you have to type “drop book” twice for it to work, “take paper” 3 times, and “drop paper” 4 times. You should store the input in a String variable and then test against that variable:


String input = UserInput.getInput("");

if(input.equalsIgnoreCase("....")) {
    ....
}
else if(input.equalsIgnoreCase("....")) {
    ....
}
//etc....
#3

huh…
when i do that it gives me this

Exception in thread "main" java.lang.StackOverflowError
	at java.util.ArrayList.contains(ArrayList.java:199)
	at org.watersedge.Player.start(Player.java:28)
	at org.watersedge.Player.reStart(Player.java:46)
	at org.watersedge.Player.start(Player.java:42)
	at org.watersedge.Player.reStart(Player.java:46)
	at org.watersedge.Player.start(Player.java:42)
	at org.watersedge.Player.reStart(Player.java:46)
	at org.watersedge.Player.start(Player.java:42)
	at org.watersedge.Player.reStart(Player.java:46)
	at org.watersedge.Player.start(Player.java:42)
	at org.watersedge.Player.reStart(Player.java:46)
	at org.watersedge.Player.start(Player.java:42)
	at org.watersedge.Player.reStart(Player.java:46)
       // this goes on for a long time......

I think the problem is that every time the start() method runs again,
after I assign what the input variable is… It just keeps on looping and
never stops because input = “take book” every time afterwards…
I have tried assigning input to null, input = null, just before the
reStart() method… But it didn’t seem to do anything…

#4

Why are you calling reStart() if it just calls start? That creates an infinite loop :slight_smile:

Could you show us the new code for all these methods?

Also, one tip, don’t create a new BufferedReader object every time in getInput(String), make it an instance variable so you can re-access it and not waste resources. Also that may possibly fix your issue.

#5

Blast! I took too much time and ra4king basically covered what I did, just using more basic terms. xD

First: Recursion is bad unless you have a reason for doing it, and I don’t think you really do. What this means, is that it’s a bad habit to have a method call another method that will call first. Especially if you have no way for them to break out of it (Not sure if you do, since reStart isn’t displayed). In this case, you’ll end up having a stack overflow in no time. My advice? If Restart is mainly performing logic that has to occur every time step, place it inside of some sort of loop instead, and include the call to start in your loop.

Second: Error line numbers aren’t very helpful if we don’t have the source/correct line numbers to match them up to. In this case, it’s line being given the an older edition of a book, then being given page numbers from the new, expanded book. We can sort of figure it out to a point, but it’s difficult.

Third: Lots of statics calls. To things that seem to be objects (Room1, which has a field room1, and UserInput). This is more of a pet peeve of mine than anything else.

Ah, to recap: Try to take care of the recursion problem, it could be a big part of your troubles, or it could be obscuring what the real problem is. If the error is popping up in code we don’t have, paste those chunks and mark which are causing the problems in the stack trace. :smiley:

#6

just a quick scribble(if you aren’t using Java7 use a if else instead of the switch case)


...
   String[] command = getInput().split(" ");
   switch(command[0])
  {
     case "take":
       switch(room, inventory, command[1]); break;
     case "drop":
        switch(inventory, room, command[1]); break;
  }
...

private void switch(List<String> a, List<String> b, String object)
{
  if(a.remove(object))
    b.add(object);
}