3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Which Java version are you using? \$\endgroup\$
    – lealand
    Oct 18, 2015 at 18:40
  • \$\begingroup\$ @lealand Only Java 7, for android compatibility. \$\endgroup\$ Oct 18, 2015 at 18:43

2 Answers 2

2
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$
    – lealand
    Oct 21, 2015 at 3:24
3
\$\begingroup\$

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
  }
}
\$\endgroup\$
6
  • \$\begingroup\$ Curious, how would you do this in Java 8? \$\endgroup\$ Oct 18, 2015 at 18:45
  • \$\begingroup\$ 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. \$\endgroup\$
    – lealand
    Oct 18, 2015 at 18:47
  • \$\begingroup\$ You don't have to accept so soon. I suggest waiting a few days for better answers. \$\endgroup\$
    – lealand
    Oct 18, 2015 at 19:09
  • 1
    \$\begingroup\$ it might be possible to use .orElse of Optional to simplify the java 8 version \$\endgroup\$
    – Vogel612
    Oct 18, 2015 at 20:54
  • \$\begingroup\$ @Vogel612 see, I didn't even know that existed. I'll edit that in. \$\endgroup\$
    – lealand
    Oct 18, 2015 at 21:17

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.