Refactor LobbyController to make the code simpler

This change gets rid of session access and makes use of a player repository. This allows for encapsulated and testable Player management.

12 files changed, 289 insertions, 82 deletions

diff --git a/src/main/java/org/luxons/sevenwonders/actions/JoinOrCreateGameAction.java b/src/main/java/org/luxons/sevenwonders/actions/ChooseNameAction.java
index cf5596da..42a26f37 100644
--- a/src/main/java/org/luxons/sevenwonders/actions/JoinOrCreateGameAction.java
+++ b/src/main/java/org/luxons/sevenwonders/actions/ChooseNameAction.java
@@ -3,24 +3,12 @@ package org.luxons.sevenwonders.actions;
 import javax.validation.constraints.NotNull;
 import javax.validation.constraints.Size;
 
-public class JoinOrCreateGameAction {
-
-    @NotNull
-    @Size(min=2, max=30)
-    private String gameName;
+public class ChooseNameAction {
 
     @NotNull
     @Size(min=2, max=20)
     private String playerName;
 
-    public String getGameName() {
-        return gameName;
-    }
-
-    public void setGameName(String gameName) {
-        this.gameName = gameName;
-    }
-
     public String getPlayerName() {
         return playerName;
     }
diff --git a/src/main/java/org/luxons/sevenwonders/actions/CreateGameAction.java b/src/main/java/org/luxons/sevenwonders/actions/CreateGameAction.java
new file mode 100644
index 00000000..ce1783c0
--- /dev/null
+++ b/src/main/java/org/luxons/sevenwonders/actions/CreateGameAction.java
@@ -0,0 +1,19 @@
+package org.luxons.sevenwonders.actions;
+
+import javax.validation.constraints.NotNull;
+import javax.validation.constraints.Size;
+
+public class CreateGameAction {
+
+    @NotNull
+    @Size(min=2, max=30)
+    private String gameName;
+
+    public String getGameName() {
+        return gameName;
+    }
+
+    public void setGameName(String gameName) {
+        this.gameName = gameName;
+    }
+}
diff --git a/src/main/java/org/luxons/sevenwonders/actions/JoinGameAction.java b/src/main/java/org/luxons/sevenwonders/actions/JoinGameAction.java
new file mode 100644
index 00000000..82bff168
--- /dev/null
+++ b/src/main/java/org/luxons/sevenwonders/actions/JoinGameAction.java
@@ -0,0 +1,17 @@
+package org.luxons.sevenwonders.actions;
+
+import javax.validation.constraints.NotNull;
+
+public class JoinGameAction {
+
+    @NotNull
+    private Long gameId;
+
+    public Long getGameId() {
+        return gameId;
+    }
+
+    public void setGameId(Long gameId) {
+        this.gameId = gameId;
+    }
+}
diff --git a/src/main/java/org/luxons/sevenwonders/actions/ReorderPlayersAction.java b/src/main/java/org/luxons/sevenwonders/actions/ReorderPlayersAction.java
new file mode 100644
index 00000000..803a71d8
--- /dev/null
+++ b/src/main/java/org/luxons/sevenwonders/actions/ReorderPlayersAction.java
@@ -0,0 +1,18 @@
+package org.luxons.sevenwonders.actions;
+
+import java.util.List;
+import javax.validation.constraints.NotNull;
+
+public class ReorderPlayersAction {
+
+    @NotNull
+    private List<String> orderedPlayers;
+
+    public List<String> getOrderedPlayers() {
+        return orderedPlayers;
+    }
+
+    public void setOrderedPlayers(List<String> orderedPlayers) {
+        this.orderedPlayers = orderedPlayers;
+    }
+}
diff --git a/src/main/java/org/luxons/sevenwonders/actions/UpdateSettingsAction.java b/src/main/java/org/luxons/sevenwonders/actions/UpdateSettingsAction.java
new file mode 100644
index 00000000..deb5e530
--- /dev/null
+++ b/src/main/java/org/luxons/sevenwonders/actions/UpdateSettingsAction.java
@@ -0,0 +1,19 @@
+package org.luxons.sevenwonders.actions;
+
+import javax.validation.constraints.NotNull;
+
+import org.luxons.sevenwonders.game.Settings;
+
+public class UpdateSettingsAction {
+
+    @NotNull
+    private Settings settings;
+
+    public Settings getSettings() {
+        return settings;
+    }
+
+    public void setSettings(Settings settings) {
+        this.settings = settings;
+    }
+}
diff --git a/src/main/java/org/luxons/sevenwonders/controllers/LobbyController.java b/src/main/java/org/luxons/sevenwonders/controllers/LobbyController.java
index 285fd778..cfc540bb 100644
--- a/src/main/java/org/luxons/sevenwonders/controllers/LobbyController.java
+++ b/src/main/java/org/luxons/sevenwonders/controllers/LobbyController.java
@@ -4,21 +4,25 @@ import java.security.Principal;
 import java.util.Collection;
 import java.util.Collections;
 
-import org.luxons.sevenwonders.actions.JoinOrCreateGameAction;
+import org.luxons.sevenwonders.actions.ChooseNameAction;
+import org.luxons.sevenwonders.actions.CreateGameAction;
+import org.luxons.sevenwonders.actions.JoinGameAction;
+import org.luxons.sevenwonders.actions.ReorderPlayersAction;
 import org.luxons.sevenwonders.actions.StartGameAction;
+import org.luxons.sevenwonders.actions.UpdateSettingsAction;
 import org.luxons.sevenwonders.errors.ApiMisuseException;
 import org.luxons.sevenwonders.game.Game;
 import org.luxons.sevenwonders.game.Lobby;
 import org.luxons.sevenwonders.game.Player;
 import org.luxons.sevenwonders.repositories.GameRepository;
 import org.luxons.sevenwonders.repositories.LobbyRepository;
-import org.luxons.sevenwonders.session.SessionAttributes;
+import org.luxons.sevenwonders.repositories.PlayerRepository;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.messaging.handler.annotation.MessageMapping;
 import org.springframework.messaging.handler.annotation.SendTo;
-import org.springframework.messaging.simp.SimpMessageHeaderAccessor;
+import org.springframework.messaging.simp.SimpMessagingTemplate;
 import org.springframework.messaging.simp.annotation.SendToUser;
 import org.springframework.messaging.simp.annotation.SubscribeMapping;
 import org.springframework.stereotype.Controller;
@@ -33,80 +37,126 @@ public class LobbyController {
 
     private final GameRepository gameRepository;
 
+    private final PlayerRepository playerRepository;
+
+    private final SimpMessagingTemplate template;
+
     @Autowired
-    public LobbyController(LobbyRepository lobbyRepository, GameRepository gameRepository) {
+    public LobbyController(LobbyRepository lobbyRepository, GameRepository gameRepository,
+            PlayerRepository playerRepository, SimpMessagingTemplate template) {
         this.lobbyRepository = lobbyRepository;
         this.gameRepository = gameRepository;
+        this.playerRepository = playerRepository;
+        this.template = template;
+    }
+
+    @MessageMapping("/chooseName")
+    public void chooseName(@Validated ChooseNameAction action, Principal principal) {
+        String userName = principal.getName();
+        Player player = playerRepository.updateOrCreatePlayer(userName, action.getPlayerName());
+
+        logger.info("Player '{}' chose the name '{}'", userName, player.getDisplayName());
     }
 
     @SubscribeMapping("/games") // prefix /topic not shown
-    public Collection<Lobby> listGames() {
-        logger.info("Subscribed to /games");
+    public Collection<Lobby> listGames(Principal principal) {
+        logger.info("Player '{}' subscribed to /topic/games", principal.getName());
         return lobbyRepository.list();
     }
 
-    @MessageMapping("/lobby/create-game")
+    @MessageMapping("/lobby/create")
     @SendTo("/topic/games")
-    public Collection<Lobby> createGame(SimpMessageHeaderAccessor headerAccessor,
-            @Validated JoinOrCreateGameAction action, Principal principal) {
-        checkThatUserIsNotInAGame(headerAccessor, "cannot create another game");
+    public Collection<Lobby> createGame(@Validated CreateGameAction action, Principal principal) {
+        checkThatUserIsNotInAGame(principal, "cannot create another game");
 
-        Player gameOwner = new Player(action.getPlayerName(), principal.getName());
+        Player gameOwner = playerRepository.find(principal.getName());
         Lobby lobby = lobbyRepository.create(action.getGameName(), gameOwner);
-        headerAccessor.getSessionAttributes().put(SessionAttributes.ATTR_LOBBY, lobby);
+        gameOwner.setLobby(lobby);
 
-        logger.info("Game '{}' (id={}) created by {} ({})", lobby.getName(), lobby.getId(), gameOwner.getDisplayName(),
+        logger.info("Game '{}' ({}) created by {} ({})", lobby.getName(), lobby.getId(), gameOwner.getDisplayName(),
                 gameOwner.getUserName());
         return Collections.singletonList(lobby);
     }
 
-    @MessageMapping("/lobby/join-game")
-    @SendToUser("/queue/join-game")
-    public Collection<Lobby> joinGame(SimpMessageHeaderAccessor headerAccessor,
-            @Validated JoinOrCreateGameAction action, Principal principal) {
-        checkThatUserIsNotInAGame(headerAccessor, "cannot join another game");
+    @MessageMapping("/lobby/join")
+    @SendToUser("/queue/lobby")
+    public Collection<Lobby> joinGame(@Validated JoinGameAction action, Principal principal) {
+        checkThatUserIsNotInAGame(principal, "cannot join another game");
 
-        Lobby lobby = lobbyRepository.find(action.getGameName());
-        Player newPlayer = new Player(action.getPlayerName(), principal.getName());
+        Lobby lobby = lobbyRepository.find(action.getGameId());
+        Player newPlayer = playerRepository.find(principal.getName());
         lobby.addPlayer(newPlayer);
-        headerAccessor.getSessionAttributes().put(SessionAttributes.ATTR_LOBBY, lobby);
+        newPlayer.setLobby(lobby);
 
-        logger.info("Player {} joined game {}", action.getPlayerName(), action.getGameName());
+        logger.info("Player '{}' ({}) joined game {}", newPlayer.getDisplayName(), newPlayer.getUserName(),
+                lobby.getName());
         return Collections.singletonList(lobby);
     }
 
-    private void checkThatUserIsNotInAGame(SimpMessageHeaderAccessor headerAccessor,
-            String impossibleActionDescription) {
-        Lobby lobby = (Lobby)headerAccessor.getSessionAttributes().get(SessionAttributes.ATTR_LOBBY);
+    private void checkThatUserIsNotInAGame(Principal principal, String impossibleActionDescription) {
+        Lobby lobby = playerRepository.find(principal.getName()).getLobby();
         if (lobby != null) {
             throw new UserAlreadyInGameException(lobby.getName(), impossibleActionDescription);
         }
     }
 
-    @MessageMapping("/lobby/start-game")
-    public void startGame(SimpMessageHeaderAccessor headerAccessor, @Validated StartGameAction action,
-            Principal principal) {
-        Lobby lobby = getOwnedLobby(headerAccessor, principal);
-        Game game = lobby.startGame(action.getSettings());
+    @MessageMapping("/lobby/reorderPlayers")
+    public void reorderPlayers(@Validated ReorderPlayersAction action, Principal principal) {
+        Lobby lobby = getLobby(principal);
+        lobby.reorderPlayers(action.getOrderedPlayers());
+
+        logger.info("Players in game {} reordered to {}", lobby.getName(), action.getOrderedPlayers());
+        sendLobbyUpdateToPlayers(lobby);
+    }
+
+    @MessageMapping("/lobby/updateSettings")
+    public void updateSettings(@Validated UpdateSettingsAction action, Principal principal) {
+        Lobby lobby = getLobby(principal);
+        lobby.setSettings(action.getSettings());
+
+        logger.info("Updated settings of game {}", lobby.getName());
+        sendLobbyUpdateToPlayers(lobby);
+    }
+
+    private void sendLobbyUpdateToPlayers(Lobby lobby) {
+        template.convertAndSend("/topic/lobby/" + lobby.getId() + "/updated", lobby);
+    }
+
+    @MessageMapping("/lobby/start")
+    public void startGame(@Validated StartGameAction action, Principal principal) {
+        Lobby lobby = getOwnedLobby(principal);
+        Game game = lobby.startGame();
         gameRepository.add(game);
 
         logger.info("Game {} successfully started", game.getId());
+        template.convertAndSend("/topic/lobby/" + lobby.getId() + "/started", (Object)null);
     }
 
-    private Lobby getOwnedLobby(SimpMessageHeaderAccessor headerAccessor, Principal principal) {
-        Lobby lobby = (Lobby)headerAccessor.getSessionAttributes().get(SessionAttributes.ATTR_LOBBY);
-        if (lobby == null) {
-            throw new UserOwnsNoLobbyException("User " + principal.getName() + " is not in a lobby");
-        }
+    private Lobby getOwnedLobby(Principal principal) {
+        Lobby lobby = getLobby(principal);
         if (!lobby.isOwner(principal.getName())) {
-            throw new UserOwnsNoLobbyException("User " + principal.getName() + " does not own the lobby he's in");
+            throw new UserIsNotOwnerException(principal.getName());
         }
         return lobby;
     }
 
-    private static class UserOwnsNoLobbyException extends ApiMisuseException {
-        UserOwnsNoLobbyException(String message) {
-            super(message);
+    private Lobby getLobby(Principal principal) {
+        Lobby lobby = playerRepository.find(principal.getName()).getLobby();
+        if (lobby == null) {
+            throw new UserNotInLobbyException(principal.getName());
+        }
+        return lobby;
+    }
+
+    private static class UserNotInLobbyException extends ApiMisuseException {
+        UserNotInLobbyException(String userName) {
+            super("User " + userName + " is not in a lobby, create or join a game first");
+        }
+    }
+
+    private static class UserIsNotOwnerException extends ApiMisuseException {
+        UserIsNotOwnerException(String userName) {
+            super("User " + userName + " does not own the lobby he's in");
         }
     }
 
diff --git a/src/main/java/org/luxons/sevenwonders/game/Lobby.java b/src/main/java/org/luxons/sevenwonders/game/Lobby.java
index b115d32e..525321b2 100644
--- a/src/main/java/org/luxons/sevenwonders/game/Lobby.java
+++ b/src/main/java/org/luxons/sevenwonders/game/Lobby.java
@@ -2,7 +2,6 @@ package org.luxons.sevenwonders.game;
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.stream.Collectors;
 
 import org.luxons.sevenwonders.game.data.GameDefinition;
 
@@ -16,7 +15,9 @@ public class Lobby {
 
     private final GameDefinition gameDefinition;
 
-    private List<Player> players;
+    private final List<Player> players;
+
+    private Settings settings;
 
     private State state = State.LOBBY;
 
@@ -26,6 +27,7 @@ public class Lobby {
         this.owner = owner;
         this.gameDefinition = gameDefinition;
         this.players = new ArrayList<>(gameDefinition.getMinPlayers());
+        this.settings = new Settings();
         players.add(owner);
     }
 
@@ -41,7 +43,15 @@ public class Lobby {
         return players;
     }
 
-    public synchronized int addPlayer(Player player) throws GameAlreadyStartedException, PlayerOverflowException {
+    public Settings getSettings() {
+        return settings;
+    }
+
+    public void setSettings(Settings settings) {
+        this.settings = settings;
+    }
+
+    public synchronized void addPlayer(Player player) throws GameAlreadyStartedException, PlayerOverflowException {
         if (hasStarted()) {
             throw new GameAlreadyStartedException();
         }
@@ -51,9 +61,8 @@ public class Lobby {
         if (playerNameAlreadyUsed(player.getDisplayName())) {
             throw new PlayerNameAlreadyUsedException(player.getDisplayName());
         }
-        int playerId = players.size();
+        player.setIndex(players.size());
         players.add(player);
-        return playerId;
     }
 
     private boolean hasStarted() {
@@ -68,7 +77,7 @@ public class Lobby {
         return players.stream().anyMatch(p -> p.getDisplayName().equals(name));
     }
 
-    public synchronized Game startGame(Settings settings) throws PlayerUnderflowException {
+    public synchronized Game startGame() throws PlayerUnderflowException {
         if (!hasEnoughPlayers()) {
             throw new PlayerUnderflowException();
         }
@@ -82,10 +91,16 @@ public class Lobby {
     }
 
     public void reorderPlayers(List<String> orderedUserNames) {
-        players = orderedUserNames.stream().map(this::getPlayer).collect(Collectors.toList());
+        List<Player> formerList = new ArrayList<>(players);
+        players.clear();
+        for (int i = 0; i < orderedUserNames.size(); i++) {
+            Player player = getPlayer(formerList, orderedUserNames.get(i));
+            players.add(player);
+            player.setIndex(i);
+        }
     }
 
-    private Player getPlayer(String userName) {
+    private static Player getPlayer(List<Player> players, String userName) {
         return players.stream()
                       .filter(p -> p.getUserName().equals(userName))
                       .findAny()
diff --git a/src/main/java/org/luxons/sevenwonders/game/Player.java b/src/main/java/org/luxons/sevenwonders/game/Player.java
index d550cf60..3e25cb80 100644
--- a/src/main/java/org/luxons/sevenwonders/game/Player.java
+++ b/src/main/java/org/luxons/sevenwonders/game/Player.java
@@ -1,26 +1,40 @@
 package org.luxons.sevenwonders.game;
 
-public class Player {
+import com.fasterxml.jackson.annotation.JsonIgnore;
 
-    private final String displayName;
+public class Player {
 
     private final String userName;
 
+    private String displayName;
+
     private int index;
 
-    public Player(String displayName, String userName) {
-        this.displayName = displayName;
+    private transient Lobby lobby;
+
+    private transient Game game;
+
+    public Player(String userName) {
         this.userName = userName;
     }
 
-    public String getDisplayName() {
-        return displayName;
+    public Player(String displayName, String userName) {
+        this(userName);
+        setDisplayName(displayName);
     }
 
     public String getUserName() {
         return userName;
     }
 
+    public String getDisplayName() {
+        return displayName;
+    }
+
+    public void setDisplayName(String displayName) {
+        this.displayName = displayName;
+    }
+
     public int getIndex() {
         return index;
     }
@@ -28,4 +42,22 @@ public class Player {
     public void setIndex(int index) {
         this.index = index;
     }
+
+    @JsonIgnore
+    public Lobby getLobby() {
+        return lobby;
+    }
+
+    public void setLobby(Lobby lobby) {
+        this.lobby = lobby;
+    }
+
+    @JsonIgnore
+    public Game getGame() {
+        return game;
+    }
+
+    public void setGame(Game game) {
+        this.game = game;
+    }
 }
diff --git a/src/main/java/org/luxons/sevenwonders/game/Settings.java b/src/main/java/org/luxons/sevenwonders/game/Settings.java
index 7159670d..ae85fc46 100644
--- a/src/main/java/org/luxons/sevenwonders/game/Settings.java
+++ b/src/main/java/org/luxons/sevenwonders/game/Settings.java
@@ -6,6 +6,8 @@ import java.util.Random;
 
 import org.luxons.sevenwonders.game.data.definitions.WonderSide;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
 public class Settings {
 
     private int nbPlayers = -1;
@@ -30,6 +32,7 @@ public class Settings {
         wonPointsPerVictoryPerAge.put(3, 5);
     }
 
+    @JsonIgnore
     public int getNbPlayers() {
         if (nbPlayers < 0) {
             throw new IllegalStateException("The number of players has not been initialized");
@@ -97,6 +100,7 @@ public class Settings {
         this.randomSeedForTests = randomSeedForTests;
     }
 
+    @JsonIgnore
     public Random getRandom() {
         return randomSeedForTests > 0 ? new Random(randomSeedForTests) : new Random();
     }
diff --git a/src/main/java/org/luxons/sevenwonders/repositories/LobbyRepository.java b/src/main/java/org/luxons/sevenwonders/repositories/LobbyRepository.java
index 21348890..3561161b 100644
--- a/src/main/java/org/luxons/sevenwonders/repositories/LobbyRepository.java
+++ b/src/main/java/org/luxons/sevenwonders/repositories/LobbyRepository.java
@@ -42,14 +42,6 @@ public class LobbyRepository {
         return lobby;
     }
 
-    public Lobby find(String gameName) {
-        Lobby lobby = lobbies.get(gameName);
-        if (lobby == null) {
-            throw new LobbyNotFoundException(gameName);
-        }
-        return lobby;
-    }
-
     public Lobby find(long lobbyId) {
         Lobby lobby = lobbiesById.get(lobbyId);
         if (lobby == null) {
diff --git a/src/main/java/org/luxons/sevenwonders/repositories/PlayerRepository.java b/src/main/java/org/luxons/sevenwonders/repositories/PlayerRepository.java
new file mode 100644
index 00000000..e671088f
--- /dev/null
+++ b/src/main/java/org/luxons/sevenwonders/repositories/PlayerRepository.java
@@ -0,0 +1,61 @@
+package org.luxons.sevenwonders.repositories;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.luxons.sevenwonders.errors.ApiMisuseException;
+import org.luxons.sevenwonders.game.Player;
+import org.springframework.stereotype.Repository;
+
+@Repository
+public class PlayerRepository {
+
+    private Map<String, Player> players = new HashMap<>();
+
+    public Player updateOrCreatePlayer(String userName, String displayName) {
+        if (players.containsKey(userName)) {
+            return updatePlayerName(userName, displayName);
+        } else {
+            return createPlayer(userName, displayName);
+        }
+    }
+
+    private Player createPlayer(String userName, String displayName) {
+        Player player = new Player(displayName, userName);
+        add(player);
+        return player;
+    }
+
+    private void add(Player player) {
+        if (players.containsKey(player.getUserName())) {
+            throw new PlayerAlreadyExistsException(player.getUserName());
+        }
+        players.put(player.getUserName(), player);
+    }
+
+    private Player updatePlayerName(String userName, String displayName) {
+        Player player = find(userName);
+        player.setDisplayName(displayName);
+        return player;
+    }
+
+    public Player find(String userName) {
+        Player player = players.get(userName);
+        if (player == null) {
+            throw new PlayerNotFoundException(userName);
+        }
+        return player;
+    }
+
+    public static class PlayerNotFoundException extends ApiMisuseException {
+        PlayerNotFoundException(String userName) {
+            super("Player '" + userName + "' doesn't exist");
+        }
+    }
+
+    private static class PlayerAlreadyExistsException extends ApiMisuseException {
+        PlayerAlreadyExistsException(String userName) {
+            super("Player '" + userName + "' already exists");
+        }
+    }
+}
diff --git a/src/main/java/org/luxons/sevenwonders/session/SessionAttributes.java b/src/main/java/org/luxons/sevenwonders/session/SessionAttributes.java
deleted file mode 100644
index 8c69d8c3..00000000
--- a/src/main/java/org/luxons/sevenwonders/session/SessionAttributes.java
+++ /dev/null
@@ -1,8 +0,0 @@
-package org.luxons.sevenwonders.session;
-
-public class SessionAttributes {
-
-    public static final String ATTR_LOBBY = "lobby";
-
-    public static final String ATTR_GAME = "game";
-}