diff --git a/src/net/socialgamer/cah/data/ConnectedUsers.java b/src/net/socialgamer/cah/data/ConnectedUsers.java index 0bef3b0..21f66c3 100644 --- a/src/net/socialgamer/cah/data/ConnectedUsers.java +++ b/src/net/socialgamer/cah/data/ConnectedUsers.java @@ -71,18 +71,24 @@ public class ConnectedUsers { } /** - * Add a new user. - * - * @param user - * User to add. + * Checks to see if a user with the specified nickname already exists, and if not add the user, + * as an atomic operation. + * @param user User to add. {@code getNickname()} is used to determine the nickname. + * @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); - final HashMap data = new HashMap(); - data.put(LongPollResponse.EVENT, LongPollEvent.NEW_PLAYER.toString()); - data.put(LongPollResponse.NICKNAME, user.getNickname()); - broadcastToAll(MessageType.PLAYER_EVENT, data); + if (this.hasUser(user.getNickname())) { + return false; + } else { + users.put(user.getNickname().toLowerCase(), user); + final HashMap data = new HashMap(); + 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 broadcastTo, final MessageType type, final HashMap masterData) { + // TODO I think this synchronized block is pointless. synchronized (users) { for (final User u : broadcastTo) { @SuppressWarnings("unchecked") diff --git a/src/net/socialgamer/cah/handlers/RegisterHandler.java b/src/net/socialgamer/cah/handlers/RegisterHandler.java index 079140e..b608e14 100644 --- a/src/net/socialgamer/cah/handlers/RegisterHandler.java +++ b/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); - // There is a findbugs warning on this line: - // cah/src/net/socialgamer/cah/handlers/RegisterHandler.java:85 Store of non serializable - // net.socialgamer.cah.data.User into HttpSession in - // 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 - // 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. - session.setAttribute(SessionAttribute.USER, user); + if (users.checkAndAdd(user)) { + // There is a findbugs warning on this line: + // cah/src/net/socialgamer/cah/handlers/RegisterHandler.java:85 Store of non serializable + // net.socialgamer.cah.data.User into HttpSession in + // 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 + // 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. + session.setAttribute(SessionAttribute.USER, user); - data.put(AjaxResponse.NICKNAME, nick); + data.put(AjaxResponse.NICKNAME, nick); + } else { + return error(ErrorCode.NICK_IN_USE); + } } } return data;