13
\$\begingroup\$

I've been building an MMO in Java for a game that will have clients built with libGDX. I have already built clients for the browser, desktop, iOS, and Android. To accommodate multiple platforms, websockets are used, and all messages sent back and forth are in the form of strings.

The purpose of the scheduled refreshAllClients() method is to try to update all clients at a 100ms delay. I have heard that this is a good approach for an MMO, but I am not totally sure about this.

I have removed some of the code (mostly password handling and the saving and loading of game and player data) so that it is more concise.

I would like to hear about the readability and overall organization of the code. This is the largest project that I have written in Java so far, and I believe I am still not following all the best practices or taking advantage of all the language features. If you see any architectural failings, those would be great to hear about too.

If you would like to see any other methods, please let me know. The main function of the BZLogger class is to log to a file while optionally allowing to log to the console. The websocketServer class has listeners for things like onMessage or onOpen, but everything is forwarded to the Server. I'm using the java_websocket library.

public class Server {

    private final ServerSockets websocketServer;

    private final int maxIpConnectionsThreshold = 10;

    private ArrayList<Client> clients = new ArrayList<Client>();
    private HashMap<String, Integer> recentIpAddresses = new HashMap<String, Integer>();

    private MainGame game;

    public boolean isLoadingOrSaving = false;

    private BZLogger messagesRecievedLogger = new BZLogger("messagesReceived", "messagesReceived.log", false);
    private BZLogger messagesSentLogger = new BZLogger("messagesSent", "messagesSent.log", false);
    private BZLogger systemMessagesLogger = new BZLogger("systemMessages", "systemMessages.log", true);

    /**
     * When the backupCount reaches X, the player and world data will be copied to a different folder
     */
    private int backupCount = 19; //start at 19 so that it will backup early

    public Server() {
        this.systemMessagesLogger.log("Server started");

        this.websocketServer = new ServerSockets(this, 9999);
        this.websocketServer.start();

        this.game = new MainGame();
        this.game.setServer(this);
        this.systemMessagesLogger.log("Game started");

        try {
            this.loadWorld();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        this.scheduleIpRefresh();
        this.scheduleGameUpdate();
        this.scheduleClientRefresh();
        this.scheduleWorldSave();
    }
    private void scheduleIpRefresh() {
        Runnable updateIpList = new Runnable() {
            public void run() {
                Server.this.ipRefresh();
            }
        };
        int initialDelayToStart = 0;
        int timeBetweenUpdates = 3;
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(updateIpList, initialDelayToStart, timeBetweenUpdates, TimeUnit.MINUTES);
    }
    private void scheduleGameUpdate() {
        Runnable updateGame = new Runnable() {
            public void run() {
                Server.this.game.updateWorld();
            }
        };
        int initialDelayToStart = 0;
        int timeBetweenUpdates = 15;
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(updateGame, initialDelayToStart, timeBetweenUpdates, TimeUnit.MINUTES);
    }
    private void scheduleClientRefresh() {
        Runnable clientRefresh = new Runnable() {
            public void run() {
                Server.this.refreshAllClients();
            }
        };
        int initialDelayToStart = 1000;
        int timeBetweenUpdates = 100;
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(clientRefresh, initialDelayToStart, timeBetweenUpdates, TimeUnit.MILLISECONDS);
    }
    private void scheduleWorldSave() {
        Runnable worldSave = new Runnable() {
            public void run() {
                try {
                    Server.this.saveWorld();
                } catch (IOException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        };
        int initialDelayToStart = 20;
        int timeBetweenUpdates = 600;
        ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(worldSave, initialDelayToStart, timeBetweenUpdates, TimeUnit.SECONDS);
    }

    /**
     * Used by the game to send messages to players
     */
    public ArrayList<Client> getClients() {
        return this.clients;
    }

    /**
     * For every connected client, gets the messages from their array
     * Combines all the messages together into one big message
     * Sends it to that client
     */
    private void refreshAllClients() {

        if (this.isLoadingOrSaving) {
            return;
        }

        for (Client client : this.clients) {
            StringBuilder bigMessageBuilder = new StringBuilder();
            for (Message message : client.getAllMessages()) {
                bigMessageBuilder.append(message.getMessageString()).append(Message.delimiter);
            }
            if (!bigMessageBuilder.toString().equals("")) {
                client.connection.send(bigMessageBuilder.toString());
                this.messagesSentLogger.log(("sending client " + client.getPlayer().getName() + ": " + bigMessageBuilder.toString()));
            }
        }
    }

    /**
     * This method will call the ip frequency check
     * The connection will be closed if it has tried to connect too frequently
     */
    public void clientConnected(WebSocket conn, ClientHandshake handshake) {
        this.systemMessagesLogger.log("Client opened connection " + conn.getRemoteSocketAddress());
        this.processIpAddress(conn.getRemoteSocketAddress());
        this.systemMessagesLogger.log("total ips connected = " + String.valueOf(this.recentIpAddresses.size()));
        if (!this.hasIpConnectedTooFrequently(conn.getRemoteSocketAddress())) {        
            this.clients.add(new Client(conn, handshake));
            this.sendWelcome(conn);
        } else {
            conn.close(0);
            this.clientDisconnected(conn);
        }
    }
    private void sendWelcome(WebSocket conn) {
        WelcomeMessage welcome = new WelcomeMessage();
        conn.send(welcome.getMessageString());
        this.messagesSentLogger.log("sent conn welcome " + conn.getRemoteSocketAddress()) ;
    }

    /**
     * Saves the player data if it is not null
     */
    public void clientDisconnected(WebSocket conn) {
        this.systemMessagesLogger.log("Client closed connection " + conn.getRemoteSocketAddress());
        Client clientToRemove = null;
        for (Client client : this.clients) {
            if (conn.equals(client.connection)) {
                clientToRemove = client;
            }
        }
        if (clientToRemove != null) {
            if (clientToRemove.getUserName() != null && clientToRemove.getPlayer() != null) {
                try {
                    this.savePlayer(clientToRemove);
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
            this.clients.remove(clientToRemove);
        }
    }

    /**
     * Place where all messages are parsed and handled or passed to the game
     * Only messages from currently connected clients will be handled
     * For most messages, the client must be attached so that messages will be properly handled
     */
    public void processMessage(WebSocket conn, String message) {
        for (Client client : this.clients) {
            if (conn.equals(client.connection)) {

                this.messagesRecievedLogger.log("conn " + conn.getRemoteSocketAddress() + " sent:" + message);

                String delimiter = Message.delimiter;
                String[] messageFrags = message.split(delimiter);

                if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_REGISTRATION_MESSAGE.id()))) {
                    PlayerRegistrationMessage registrationMessage = PlayerRegistrationMessage.decodeMessage(messageFrags[1]);
                    this.processPlayerRegistration(registrationMessage, client);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_LOGIN_MESSAGE.id()))) {
                    PlayerLoginMessage loginMessage = PlayerLoginMessage.decodeMessage(messageFrags[1]);
                    this.processPlayerLogin(loginMessage, client);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_GAME_READY_MESSAGE.id()))) {
                    this.sendStartingPlayerAndRegionData(client);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_MOVED_MESSAGE.id()))) {
                    PlayerMovedMessage decodedMessage = PlayerMovedMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_BUILT_MESSAGE.id()))) {
                    PlayerBuiltMessage decodedMessage = PlayerBuiltMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_SOLD_MESSAGE.id()))) {
                    PlayerSoldMessage decodedMessage = PlayerSoldMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_BOUGHT_MESSAGE.id()))) {
                    PlayerBoughtMessage decodedMessage = PlayerBoughtMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_BOUGHT_ALL_MESSAGE.id()))) {
                    PlayerBoughtAllMessage decodedMessage = PlayerBoughtAllMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_TOOK_MESSAGE.id()))) {
                    PlayerTookMessage decodedMessage = PlayerTookMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_GAVE_ALL_MESSAGE.id()))) {
                    PlayerGaveAllMessage decodedMessage = PlayerGaveAllMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_GAVE_MESSAGE.id()))) {
                    PlayerGaveMessage decodedMessage = PlayerGaveMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_TOOK_REGION_MESSAGE.id()))) {
                    PlayerTookEntireRegionMessage decodedMessage = PlayerTookEntireRegionMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_ADDED_ENERGY_MESSAGE.id()))) {
                    PlayerAddedEnergyMessage decodedMessage = PlayerAddedEnergyMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_ADDED_MAX_ENERGY_MESSAGE.id()))) {
                    PlayerAddedMaxEnergyMessage decodedMessage = PlayerAddedMaxEnergyMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_ADDED_ENERGY_REGION_MESSAGE.id()))) {
                    PlayerAddedEnergyRegionMessage decodedMessage = PlayerAddedEnergyRegionMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_UPDATE_MESSAGE.id()))) {
                    PlayerUpdateMessage decodedMessage = PlayerUpdateMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_BOUGHT_ABILITY_MESSAGE.id()))) {
                    PlayerBoughtAbilityMessage decodedMessage = PlayerBoughtAbilityMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                } else if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_ACTIVATED_ABILITY_MESSAGE.id()))) {
                    PlayerActivatedAbilityMessage decodedMessage = PlayerActivatedAbilityMessage.decodeMessage(messageFrags[1]);
                    decodedMessage.client = client;
                    this.game.acceptMessage(decodedMessage);
                }
            }
        }
    }

    /**
     * To allow for logins from multiple locations/devices
     */
    private void tryToRemoveExistingPlayer(String name) {
        Client clientToRemove = null;
        for (Client existingClient : this.clients) {
            if (existingClient.getPlayer() != null &&
                name.equals(existingClient.getPlayer().getName())) {
                this.systemMessagesLogger.log("found matching player " + existingClient.getPlayer().getName());
                clientToRemove = existingClient;
            }
        }
        if (clientToRemove != null) {
            clientToRemove.connection.close(0);
            this.clientDisconnected(clientToRemove.connection);
        }
    }

    /**
     * This is the initial payload sent to the client that contains the nearby regions, and player data
     */
    private void sendStartingPlayerAndRegionData(Client client) {
        if (client.getPlayer() == null) {
            String userName = client.getUserName();
            if (userName != null) {
                Player savedPlayer = this.loadPlayerForUserName(userName);
                if (savedPlayer == null) {
                    return;
                }
                client.setPlayer(savedPlayer);
            }
        }

        PlayerMessage playerMessage = new PlayerMessage(client.getPlayer().encodeToString());
        client.connection.send(playerMessage.getMessageString());

        for (Region region : this.game.getClosebyRegionsForClient(client)) {
            RegionMessage regionMessage = new RegionMessage(region.encodeToString(), region.worldPosition());
            client.connection.send(regionMessage.getMessageString());
        }

        this.systemMessagesLogger.log("adding client " + client.getPlayer().getName());
    }

    /**
     * If the current count for an ip address in the map is over the threshold,
     * Reduces the count back to the threshold
     * Otherwise simply reduces the currently saved count by 2
     * Over time this will allow ip addresses to try to connect again
     */
    private void ipRefresh() {
        for (String string : this.recentIpAddresses.keySet()) {
            int persistentlyConnectingThreshold = 12;
            int currentCount = this.recentIpAddresses.get(string);
            if (currentCount > persistentlyConnectingThreshold) {
                this.recentIpAddresses.put(string, 10);
            } else {
                this.recentIpAddresses.put(string, Math.max(this.recentIpAddresses.get(string) - 2, 0));
            }
        }
    }

    /**
     * Adds the ip address to the map of recently connected IPs
     * Also keeps track of the number of times it has connected (if already present in map)
     */
    private void processIpAddress(InetSocketAddress address) {
        boolean wasInMap = false;
        for (String string : this.recentIpAddresses.keySet()) {
            if (string.equals(address.getHostString())) {
                int currentCount = this.recentIpAddresses.get(string);
                this.recentIpAddresses.put(string, currentCount + 1);
                wasInMap = true;
            }
        }
        if (!wasInMap) {
            this.recentIpAddresses.put(address.getHostString(), 1);
        }

        this.systemMessagesLogger.log("num times connected = " + String.valueOf(this.recentIpAddresses.get(address.getHostString())));
    }

    private boolean hasIpConnectedTooFrequently(InetSocketAddress address) {
        int count = this.recentIpAddresses.get(address.getHostString());
        if (count > this.maxIpConnectionsThreshold) {
            return true;
        }
        return false;
    }
}

Play the game here

\$\endgroup\$
  • \$\begingroup\$ That's a pretty cool game, I think it has a lot of potential too. Good game! \$\endgroup\$ – Ethan Bierlein May 7 '15 at 13:36
  • 1
    \$\begingroup\$ Really glad you like it! Thanks for giving it a try. \$\endgroup\$ – bazola May 7 '15 at 14:17
  • 1
    \$\begingroup\$ Should this line private final ServerSockets websocketServer; be private final ServerSocket websocketServer; \$\endgroup\$ – MaxZoom May 7 '15 at 14:43
  • \$\begingroup\$ @bazola Do you have plans to make it standalone? \$\endgroup\$ – Ethan Bierlein May 7 '15 at 18:15
  • 1
    \$\begingroup\$ @EthanBierlein yes, I have working versions for desktop, iOS, and Android, however I am not sure when I will release them (I probably need some better art assets first) \$\endgroup\$ – bazola May 7 '15 at 18:57
12
\$\begingroup\$

Some quick things that I can see :

public ArrayList<Client> getClients() {
        return this.clients;
}

You're returning the implementation, you should almost always flavor the interface, in this case List. Hiding implementation is a good thing.

    try {
        this.loadWorld();
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

Auto-generated code should be changed almost immediately. Printing to the stacktrace is most of the time not how you want to manage things. If you really don't know how to manage this, log the error with the stacktrace and be done with it. A log has more chance to be look at and be saved somewhere than the console output is.

String[] messageFrags = message.split(delimiter);

                if (messageFrags[0].equals(String.valueOf(MessageType.PLAYER_REGISTRATION_MESSAGE.id())))

....

This piece of code is rather not that readable. I would first maybe create String version ofMessageType.PLAYER_REGISTRATION_MESSAGE.id() since it would remove a lot of not so useful transformation. For the if itself, you can use a switch for a String maybe it would be a solution.

\$\endgroup\$
  • \$\begingroup\$ Since this is a server, performance is also important. Using a switch really is a good idea, both for readability, debugging, maintenance and performance. (If I'm wrong, please, kindly correct me.) One thing you can add is that the port number should be defined as a property in the class. This allows to quickly change the port and can be used to (say) get/get the port that the server is using to communicate. \$\endgroup\$ – Ismael Miguel May 7 '15 at 9:08
11
\$\begingroup\$
/**
 * When the backupCount reaches X, the player and world data will be copied to a different folder
 */
private int backupCount = 19; //start at 19 so that it will backup early

Unused variable with a comment that speaks of functionality so large it probably requires it's own set of classes.

What's going on here?

Presumably, you have some code that you left out. Even then, I'd say that this is not the way to handle this kind of thing - "different folder" and "backup" sounds like an insecure type of redundancy.

And starting at 19 just because this will cause "it" to backup early sounds like a massive hack at best.


Some rants:

  • Overuse of this for referring to the current instance, it's just extra noise and makes it harder for me to distinguish between function calls on other objects and function calls on this.
  • processMessage couples your server and all message classes together - consider mapping id to class and having that process your message
  • Server is too big in general - mostly because it's dipping too deep at times. It has to do things, sure, but it doesn't need to do everything all by itself. Methods here should be high level and don't care about implementation, instead delegating this to other classes.
  • I don't see anything related to security. I worry about people sending fake messages to your server.
\$\endgroup\$
  • \$\begingroup\$ I did mention in my post that I omitted the saving and loading code. The backup happens if backupCount is greater than 20 when the world is saved. I have certain reasons for making a backup copy of the world and player data, but they will be the subject of another question. \$\endgroup\$ – bazola May 6 '15 at 14:35
10
\$\begingroup\$

You will gain noticeable performance and code clarity by using switch statement instead if()..else if(..) in processMessage method.

I would also define constants as the class static final variables instead defining them per each call in method (or defined them in config file if preferable). It is easier to control and maintain them if define in one place.

Method tryToRemoveExistingPlayer should return boolean or object to indicate its result.

Try to add a comment to describe each method. Based on the comment rename the method as some method names are a bit uncertain.

\$\endgroup\$
  • \$\begingroup\$ Code clarity, probably. Performance? Probably not. If the bulk of the work being done by the game engine is figuring out what String was passed in (considering most of them will simply stop after the first couple characters) then that's a good problem to have because that means the server load is super, super low. \$\endgroup\$ – corsiKa May 7 '15 at 14:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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