Find existing player to update or create new one

Question

I'm making a game in Java 7, and I have this block of logic when handling an update from the server.

public void handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 boolean done = false;
 for (Player otherPlayer : otherPlayers) {
 if (newPlayer.id == otherPlayer.id) {
 updatePlayer(otherPlayer);
 done = true;
 break;
 }
 }
 if (!done) {
 createPlayer(newPlayer);
 }
}

The client has a list of players, and gets a player object from the server. If the client already has that player (one with the same id), it updates that existing one, otherwise it creates a new one.

This code looks ugly though. The done flag is really getting under my skin. Is there a better way to organize this to avoid that? Are there any other problems with this code?

Sidenote: createPlayer() and updatePlayer() do very different things, and need to be seperate methods.

• lealand: Which Java version are you using?
• Anubian Noob: @lealand Only Java 7, for android compatibility.

Answer 1

My suggestion is a bit longer, but I think it spells out the intent and flow more clearly. One can tell what each method does at a glance, instead of having to run the code mentally.

public void handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 final Player existingPlayer = findPlayerById(newPlayer.id, otherPlayers);
 if (existingPlayer == null) {
 createPlayer(newPlayer);
 } else {
 updatePlayer(existingPlayer);
 }
}
private Player findPlayerById(long id, Player[] otherPlayers) {
 for (Player player : otherPlayers) {
 if (id == player.id) {
 return player;
 }
 }
 return null;
}

• lealand: I like this solution. With Java 8 I think it could be even further improved by using an Optional<Player> as I mention in my review just because I hate seeing nulls.

Answer 2

A very quick review for the code, and assuming Java < 8:

public void handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 for (Player otherPlayer : otherPlayers) {
 if (newPlayer.id == otherPlayer.id) {
 updatePlayer(otherPlayer);
 return;
 }
 }
 createPlayer(newPlayer);
}

Some code styles prohibit this because you typically only want to have one return in a function (there's two here), but I typically overlook this for simple methods where the logic ends up clearer in my opinion.

And here's a Java 8 way (might not be optimal)

public void handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 final Optional<Player> otherPlayer = Arrays.stream(otherPlayers)
 .filter((Player p) -> newPlayer.id == p.id)
 .findFirst();
 if (otherPlayer.isPresent()) {
 updatePlayer(otherPlayer.get());
 }
 else {
 createPlayer(newPlayer);
 }
}

It doesn't do much on the LOC metric, but it has a much clearer semantic meaning.

A comment from Vogel612 brings up the Optional#orElse method. This sounds like a great method, but the only way I can see it used here is as follows:

public void handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 final Optional<Player> player = Arrays.stream(otherPlayers)
 .filter((Player p) -> newPlayer.id == p.id)
 .findFirst()
 .orElse(newPlayer);
 if (newPlayer == player) {
 createPlayer(newPlayer);
 }
 else {
 updatePlayer(player);
 }
}

I think this has even greater semantic meaning, but your mileage my vary. Additionally, I still feel like we're lacking the conciseness of the final version below.

Finally, you can now use Groovy for Android development. Here's how this can be done in Groovy:

def handleNewPlayer(Player newPlayer, Player[] otherPlayers) {
 def otherPlayer = otherPlayers.find() { newPlayer.id == otherPlayer.id }
 if (otherPlayer) {
 updatePlayer otherPlayer
 }
 else {
 createPlayer newPlayer
 }
}

• Anubian Noob: Curious, how would you do this in Java 8?
• lealand: Stream the otherPlayers array and filter. I haven't used the new API at all, but there's some lovely ways to handle loops and conditionals.
• lealand: You don't have to accept so soon. I suggest waiting a few days for better answers.
• Vogel612: it might be possible to use .orElse of Optional to simplify the java 8 version
• lealand: @Vogel612 see, I didn't even know that existed. I'll edit that in.