0

I have been looking so hard to find an answer but I do not know what I'm doing wrong. I'm learning how use JAVA threads. Thing is I'm doing a space-ship game. I have one method that creates enemys (those enemys are added to an ArrayList thats is painted to make the movement efect, that works great). I have another method that look in this ArrayList for those enemys that are dead (dead is a boolean that becomes true if the enemy disapear of the screen or its killed) and if they are (dead) it erease those from the ArrayList (to not be painted anymore). I have one thread that use the createEnemy method (it works fine). Problem comes now, I need to use this ereaseEnemy method but it gives me and concurrency error, I have tried using synchronized on both methods but the ereaseEnemy method never start to work. Don't know how fix this. SHoud I stop the first thread (creator) to make the other work? I'm I missing somethig here? Thanks!

Code that removes the enemies

for (Enemigo enemigo1 : enemigos) {
    if (!enemigo1.isEstaVivo()) { enemigos.remove(enemigo1); }
}
Improve this question
5
  • 4
    You probably shouldn't be using multiple threads at all. Commented Aug 26, 2012 at 19:35
  • 1
    Example code on how you are removing an entry fro arraylist? Commented Aug 26, 2012 at 19:37
  • ok, It will work, but I have no clue about how do it not using them anyway. It's shure that I have to use the creator thread, it's the only way I can keep coming enemys. But, as I said, don't know how can I clean that enemy list. If I don't do that list keep growing and growing. any advice? Commented Aug 26, 2012 at 19:38
  • thats the code I use to erease enemies. Sorry, but code is in spanish, anyway it's understable. for (Enemigo enemigo1 : enemigos) { if (!enemigo1.isEstaVivo()) { enemigos.remove(enemigo1); } } Commented Aug 26, 2012 at 19:40
  • @user1579122 use this Commented Aug 26, 2012 at 19:42

3 Answers 3

Reset to default
6

You really should post the offending code, but I can make an educated guess: you are iterating over an ArrayList and inside the loop you are calling list.remove(o). The exception thrown is ConcurrentModificationException. You are not allowed to call any of the List.remove() methods while iterating; you must use Iterator.remove(). That precludes the usage of the enhanced for loop for this use case. Change your code to

for (Iterator<Enemigo> iter = enemigos.iterator(); iter.hasNext();)
  if (!iter.next().isEstaVivo()) iterator.remove();
Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

I am sort of suspecting same. I guess he is calling remove(index).
@Nambari he is not using remove(index)
@veer: enemigos.remove(enemigo1); I am assuming enemigos is list and he is calling enemigos.remove(enemigo1); remove based on index. isn't it?
ok ok, then I have to read lot of things before I ask again, thak you all very much, will bac to say if I did something useful
BTW funny thing how you end up naming the method "is is alive" due to language discrepancies :)
|
1

Two possibles solutions

1) Make a copy of your list before deleting (be aware of performance issues if the size is too big)

ArrayList<enemigo> enemigosCopy= new ArrayList<enemigo>(); 
enemigosCopy.addAll(enemigos);
//Do your deleting thing on enemigosCopy

2) Use an Iterator

 Iterator i =enemigos.iterator();
 while (i.hasNext()) {
   enemigo o = i.next();
   if (!enemigo1.isEstaVivo()) {    
     i.remove(o);      
   }             
 }
Improve this answer

6 Comments

Why do deleting? He should build the new list by checking for each element whether it is to be added. for (Enemigo e : enemigos) if (e.isEstaVivo()) enemigosCopy.add(e);
ok, but I have a doubt with that, what I will erease is an enemy form de copyList, but Do I still have the original list with all of them?
Yes, the original stays untouched. Note that the rebuilding approach may actually be more performant. ArrayList.remove is really a drag, when you think about what it must be doing.
ok, but what i really need is clean the original, this is the one I use to repaint each ship to make movement, so, I don't really understand how this copy of the list will help me.
It will work only if you reassign the new list to the variable that used to hold the old one. If you reference the same list from many places, then don't use this approach.
|
0

You shouldn't be trying to synchronise the threads yourself. Let Java do that for you by using the java.util.concurrent classes. In your case I would look at the ConcurrentLinkedQueue or ConcurrentMap for constant access times. You can use a ship id as the key to the map.

You will still need to control setting the dead flag on the enemies so that it's access is handled correctly by the multiple threads. The data storage won't do that for you, it'll just make sure you have a consistent state of the data store for all threads.

http://docs.oracle.com/javase/6/docs/api/index.html?java/util/concurrent/package-summary.html

Improve this answer

1 Comment

ok, I'm going to look all over it. thaks, will back to tell if I fixed that or not.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.