-2

I'm creating an online videogame using Java. If have a client and a server application. In the server, to handle the player database, I create an ArrayList named inGamePlayers, that contains Players object (with InetAdress ipAdress and String username).

When a Player Connects, it first check if the connecting player username == to one of the Players in the ArrayList username. If not, it adds him in the list. Else, the connecting player is considered 'reconnected'...

When It runs on Eclipse, it worked weird. So I decided to put a part of my code as a test in "Java Tutor" (It's a website that reads your code and shows you the variables, really handy when you start programming).

At the first

for (Player p : inGamePlayers) {

line, it stops and says java.util.ConcurrentModificationException, not the first but the second time it passes-by.

Here's my entire test code

import java.util.ArrayList;

public class YourClassNameHere {

    public static void main(String[] args) {

        ArrayList<Player> inGamePlayers = new ArrayList<Player>();
        inGamePlayers.add(new Player("Griff"));

        String newUsername = "Polak";

        for (Player p : inGamePlayers) {

            if (newUsername == p.username) {
                System.out.println(p.username+" reconnected!");
                break;
            }

            inGamePlayers.add(new Player(newUsername));
        }

        for (Player p : inGamePlayers) {
            System.out.println(p.username);
        }
    }
}

class Player {

    public String username;

    public Player(String username) {
        this.username = username;
    }
}
Tunaki
  • 132,869
  • 46
  • 340
  • 423
GriffinBabe
  • 146
  • 1
  • 12
  • 2
    Can you please post the whole `for` loop. Obviously the error doesn't come from only that line. – Mat Nov 28 '16 at 19:26
  • Iteration over regular collections explodes with a `ConcurrentModificationException` if you modify the collection while iterating. Simple fix: Use a `Set` (like `HashSet`) and don't check, just add - the set will ensure there are no duplicates. This "insight" is all available in the javadoc. – Bohemian Nov 28 '16 at 19:27
  • @ΦXocę웃Пepeúpaツ OP doesn't even need multiple threads - AFAICT he is adding inside the loop – Bohemian Nov 28 '16 at 19:28
  • You cannot modify an `Arraylist` while iterating over it. Check the docs: https://docs.oracle.com/javase/7/docs/api/java/util/ArrayList.html Use `CopyOnWriteArrayList` – Pravin Sonawane Nov 28 '16 at 19:29
  • should we close in advance this ***if (newUsername == p.username) {*** – ΦXocę 웃 Пepeúpa ツ Nov 28 '16 at 19:35

1 Answers1

3

This code has multiple flaws. In this form it is going to add new Player instances until an entry with matching name is found. Starting an iteration, modifying a list, and continuing the iteration will throw a ConcurrentModificationException as you observed. Furthermore you compare strings with == instead of equals.

  for (Player p : inGamePlayers) {

    if (newUsername == p.username) { // This needs to be .equals
      System.out.println(p.username+" reconnected!");
      break;
    }
    // this line runs many times
    inGamePlayers.add(new Player(newUsername));
  }

Instead I suggest you to extract that code to a new function, and change the control flow:

private static void handleConnected(ArrayList<Player> inGamePlayers, String newUsername) {
  for (Player p : inGamePlayers) {
    if (newUsername.equals(p.username)) {
      System.out.println(p.username+" reconnected!");
      return; // return instead of break
    }
  }
  // we did not return, so this user is new
  inGamePlayers.add(new Player(newUsername));
}

// ...

public static void main(String[] args) {
  ArrayList<Player> inGamePlayers = new ArrayList<Player>();
  inGamePlayers.add(new Player("Griff"));

  String newUsername = "Polak";

  // Call this function in place of the old loop
  handleConnected(inGamePlayers, newUsername);

  for (Player p : inGamePlayers) {
    System.out.println(p.username);
  }
}
Tamas Hegedus
  • 28,755
  • 12
  • 63
  • 97
  • Thank you for you alterantive solution! @PravinSonawane is right, I cannot modify my ArrayList while iterating over it – GriffinBabe Nov 28 '16 at 19:42