Atomically check for existing user with requested nickname and add new user if there is not such an existing user. Fixes #26.

src/net/socialgamer/cah/data/ConnectedUsers.java

@@ -71,18 +71,24 @@ public class ConnectedUsers {
   }
 
   /**
-   * Add a new user.
+   * Checks to see if a user with the specified nickname already exists, and if not add the user,
-   * 
+   * as an atomic operation.
-   * @param user
+   * @param user User to add. {@code getNickname()} is used to determine the nickname.
-   *          User to add.
+   * @return {@code true} if the user was added, {@code false} if another user with the same name
+   * already existed.
    */
-  public void newUser(final User user) {
+  public boolean checkAndAdd(final User user) {
     synchronized (users) {
-      users.put(user.getNickname().toLowerCase(), user);
+      if (this.hasUser(user.getNickname())) {
-      final HashMap<ReturnableData, Object> data = new HashMap<ReturnableData, Object>();
+        return false;
-      data.put(LongPollResponse.EVENT, LongPollEvent.NEW_PLAYER.toString());
+      } else {
-      data.put(LongPollResponse.NICKNAME, user.getNickname());
+        users.put(user.getNickname().toLowerCase(), user);
-      broadcastToAll(MessageType.PLAYER_EVENT, data);
+        final HashMap<ReturnableData, Object> data = new HashMap<ReturnableData, Object>();
+        data.put(LongPollResponse.EVENT, LongPollEvent.NEW_PLAYER.toString());
+        data.put(LongPollResponse.NICKNAME, user.getNickname());
+        broadcastToAll(MessageType.PLAYER_EVENT, data);
+        return true;
+      }
     }
   }
 
@@ -188,6 +194,7 @@ public class ConnectedUsers {
    */
   public void broadcastToList(final Collection<User> broadcastTo, final MessageType type,
       final HashMap<ReturnableData, Object> masterData) {
+    // TODO I think this synchronized block is pointless.
     synchronized (users) {
       for (final User u : broadcastTo) {
         @SuppressWarnings("unchecked")

src/net/socialgamer/cah/handlers/RegisterHandler.java

@@ -86,22 +86,23 @@ public class RegisterHandler extends Handler {
       final String nick = request.getParameter(AjaxRequest.NICKNAME).trim();
       if (!validName.matcher(nick).matches()) {
         return error(ErrorCode.INVALID_NICK);
-      } else if (users.hasUser(nick)) {
-        return error(ErrorCode.NICK_IN_USE);
       } else {
         final User user = new User(nick, request.getRemoteAddr(),
             Constants.ADMIN_IP_ADDRESSES.contains(request.getRemoteAddr()));
-        users.newUser(user);
+        if (users.checkAndAdd(user)) {
-        // There is a findbugs warning on this line:
+          // There is a findbugs warning on this line:
-        // cah/src/net/socialgamer/cah/handlers/RegisterHandler.java:85 Store of non serializable
+          // cah/src/net/socialgamer/cah/handlers/RegisterHandler.java:85 Store of non serializable
-        // net.socialgamer.cah.data.User into HttpSession in
+          // net.socialgamer.cah.data.User into HttpSession in
-        // net.socialgamer.cah.handlers.RegisterHandler.handle(RequestWrapper, HttpSession)
+          // net.socialgamer.cah.handlers.RegisterHandler.handle(RequestWrapper, HttpSession)
-        // I am choosing to ignore this for the time being as it works under light load, and I am
+          // I am choosing to ignore this for the time being as it works under light load, and I am
-        // intending on moving away from HttpSession for storing user data in the not too distant
+          // intending on moving away from HttpSession for storing user data in the not too distant
-        // future, to fix issues with loading the game twice in the same browser.
+          // future, to fix issues with loading the game twice in the same browser.
-        session.setAttribute(SessionAttribute.USER, user);
+          session.setAttribute(SessionAttribute.USER, user);
 
-        data.put(AjaxResponse.NICKNAME, nick);
+          data.put(AjaxResponse.NICKNAME, nick);
+        } else {
+          return error(ErrorCode.NICK_IN_USE);
+        }
       }
     }
     return data;