4
\$\begingroup\$

I have implemented a basic high score server for my strategy game. The goal here was to get the high scores working for the browser based version of the game (I am using the libGDX library with GWT for this version). Unfortunately this required using websockets, which made the process more difficult than I would like.

I will (hopefully) one day extend this basic foundation into a full blown MMO version of the game.

The logical flow of what happens here is that the client opens a connection with the server, the server sends a welcome message, the client responds to that by sending its score, the server responds to that by sending its list of high scores, and when the client receives that it sends a success message and closes the connection.

Here is the server code:

public class Server {

    private static class InnerWebServer extends WebSocketServer {

        private final Server server;

        public InnerWebServer(Server server, int port) {
            super(new InetSocketAddress(port));
            this.server = server;
        }

        @Override
        public void onOpen(WebSocket conn, ClientHandshake handshake) {
            System.out.println("Client opened connection " + conn.toString());
            this.server.sendWelcome(conn);
        }

        @Override
        public void onClose(WebSocket conn, int code, String reason, boolean remote) {
            System.out.println("Client closed connection " + conn.toString());
        }

        @Override
        public void onError(WebSocket conn, Exception ex) {
        }

        @Override
        public void onMessage(WebSocket conn, String message) {
            System.out.println("received = " + message);

            if (message.equals("Success")) {
                this.server.sendGoodbye(conn);
            } else {

                //filter out malicious stuff here
                if (this.server.processScore(message)) {

                    try {
                        this.server.saveScores();
                    } catch (IOException e) {
                        System.out.println("Saving scores failed");
                    }

                    this.server.sendScores(conn);
                } else {
                    //close the connection if the received scores message cannot be parsed
                    conn.close(0);
                }                
            }
        }
    }

    private HashMap<String, Integer> scores = new HashMap<String, Integer>();
    private final InnerWebServer websocketServer;

    public Server() {
        System.out.println("Server started");

        try {
            this.loadScores();
        } catch (IOException e) {
            System.out.println("Loading scores failed");
        }

        this.websocketServer = new InnerWebServer(this, 4200);
        this.websocketServer.start();
    }

    private boolean processScore(String scoreMessage) {
        try {
            String delims = "---";
            String[] messageFragments = scoreMessage.split(delims);
            String name = messageFragments[0];
            String score = messageFragments[1];
            int scoreInt = this.parseScore(score);
            this.scores.put(name, scoreInt);
            return true;
        } catch (Exception e) {
            return false;
        }
    }

    private int parseScore(String score) {
        int scoreInt = 0;
        try {
            scoreInt = Integer.parseInt(score);
         } catch (Exception e) {
             scoreInt = 0;
         }
        return scoreInt;
    }

    private void sendWelcome(WebSocket conn) {
        System.out.println("sending welcome");
        conn.send("Welcome");
    }

    private void sendGoodbye(WebSocket conn) {
        System.out.println("sending goodbye");
        conn.send("Goodbye");
    }

    private void sendScores(WebSocket conn) {
        System.out.println("sending scores");
        String scoresList = "";
        for (String name : this.scores.keySet()) {
            scoresList = scoresList.concat(name);
            scoresList = scoresList.concat("---");
            scoresList = scoresList.concat(String.valueOf(this.scores.get(name)));
            scoresList = scoresList.concat("---");
        }
        conn.send(scoresList);
    }

    private void saveScores() throws IOException {
        FileOutputStream file = new FileOutputStream("savedScores.scores");
        DataOutputStream out = new DataOutputStream(file);
        out.writeInt(this.scores.size());
        for (String name : this.scores.keySet()) {
            BZSerializationTools.serializeString(out, name);
            out.writeInt(this.scores.get(name));
        }
        out.close();
    }

    private void loadScores() throws IOException {
        File file = new File("savedScores.scores");
        if (file.exists()) {
            FileInputStream fileStream = new FileInputStream("savedScores.scores");
            DataInputStream in = new DataInputStream(fileStream);
            int numScores = in.readInt();
            for (int i = 0; i < numScores; i++) {
                String name = BZSerializationTools.deserializeString(in);
                int score = in.readInt();
                this.scores.put(name, score);
            }
        }
    }
}

Here is the client code that sends messages to the server and receives the scores:

private void sendScore() {        
    socket.send(this.game.getPlayer().getName() 
                + "---" 
                + String.valueOf(this.game.getPlayer().getCurrentAmount(ResourceType.GOLD)));
}

private void sendSuccessMessage() {
    socket.send("Success");
}

private void getMessage(String message) {
    if (message.equals("Welcome")) {
        this.sendScore();
    } else if (message.equals("Goodbye")) {
        this.socket.close();
        //this.socket = null; //not sure if this would be good or not
    } else {
        this.processScoresList(message);
        this.sendSuccessMessage();
    }
}

private void processScoresList(String scoresMessage) {
    String delims = "---";
    String[] messageFragments = scoresMessage.split(delims);

    int index = 0;
    for (int i = 0; i < messageFragments.length; i+=2) {
        String name = messageFragments[i];
        String score = messageFragments[i+1];
        System.out.println("name = " + name + " score = " + score);

        if (index < this.scoresLabels.size()) {
            Label scoreLabel = this.scoresLabels.get(index);
            scoreLabel.setText(name + " = " + score);
        }
        index++;
    }
}

private void establishConnection() {
    //send high score to server
    switch(Gdx.app.getType()) {
       case Android:
       case Desktop:
       case Applet:
       case HeadlessDesktop:
       case iOS:
           break;
       case WebGL:
              this.socket = new Websocket("ws://127.0.0.1:4200");
              this.socket.addListener(new WebsocketListener() {
                  @Override
                  public void onClose() {
                      // do something on close
                  }
                  @Override
                  public void onMessage(String msg) {
                      HighScoreMenu.this.getMessage(msg);
                  }
                  @Override
                  public void onOpen() {
                      // do something on open
                  }
              });
              this.socket.open();
               break;
    }
}

I have deployed the jar to an Amazon EC2 server and verified that it is working.

My main concerns going forward are two fold: security and bandwidth usage. Using an Amazon server means that I would have to pay by the gigabyte for bandwidth, so I would like to reduce that as much as possible. This means that a DDOS type attack could be very costly. Even a situation where a player opens and closes the high scores menu repeatedly could get pretty costly. I have some plans in place to deal with these issues, but any feedback or advice would be much appreciated.

Another big concern is that I am using a predetermined string delimiter, which will mean I will pretty much have to prevent the user from putting that string delimiter in their name when they create it. This does not seem optimal.

I should note that I have tried to use a byte array instead of strings for this, but unfortunately I was not able to get them to work properly with the two libraries I am using.

The game is playable here at pixelpocalypse.com

\$\endgroup\$
2
\$\begingroup\$

My main concerns going forward are two fold: security and bandwidth usage. Using an Amazon server means that I would have to pay by the gigabyte for bandwidth, so I would like to reduce that as much as possible. This means that a DDOS type attack could be very costly.

Wouldn't amazon handle ddos attacks for you?

And regarding security: currently it would be extremely easy for a player to submit any highscore they want. As you say that you have some more plans for this game, that might not be ideal.

Even a situation where a player opens and closes the high scores menu repeatedly could get pretty costly. I have some plans in place to deal with these issues, but any feedback or advice would be much appreciated.

You could make players register, and require authentication when submitting/retrieving the highscore. Then you can limit the amount of retrieving.

Another big concern is that I am using a predetermined string delimiter, which will mean I will pretty much have to prevent the user from putting that string delimiter in their name when they create it. This does not seem optimal.

It's not optimal, but I think that it is acceptable. You wouldn't even have to prevent the user from using those characters though, just split by the last occurrence (or from back to front if you have more than two values later on).

Printing Serverside

System.out.println on the server doesn't make that much sense. Nobody is going to read them, and they take up resources. I'm assuming that they are debug statements, but having so many of them makes them hard to handle (will you always comment all out when deploying, and then uncomment again when debugging?)

If you want to keep it simple, you could at least extract the printing to a separate method, so that you only have to change code in one place (then you could add an if based on a config file, comment/uncomment the print, or print to a file).

Error Handling

Why are you turning a perfectly fine exception in processScore into a boolean return value? Malformed input seems like an exceptional case, so it can be handled with exceptions.

Quite often, you catch an exception, print an error message, and that's it. That doesn't seem like a good idea. I would log the errors to a file, and I would inform the user about it (add a sendError method). Just silently failing will make debugging later on quite difficult and it will also be frustrating for a user if they are not informed about failures and possible reasons for them (is the input malformed, or does the server have problems? this is something a user would probably like to know).

onError should probably also log and inform the user.

And an invalid integer should also throw an exception, not set the score to 0.

Misc

  • use TODO for stuff that is not yet done. filter out malicious stuff here sounds as if that is currently already happening, when it is not. Adding TODO will also make it a lot easier to find all todos and not forgetting any.
  • I rarely see concat used, and I think that the reason is that the code doesn't look as good as with + or StringBuilder, and also performs worse than StringBuilder.
  • extract static strings to static fields or configuration files (such as the file name which you have twice in your code, or the delimiter that also occurs more than once).
  • this.socket = null; //not sure if this would be good or not: no, it's not needed.
\$\endgroup\$
2
\$\begingroup\$

Misc

  • private HashMap<String, Integer> scores = new HashMap<String, Integer>(); You're using the implementation instead of the interface. I'm not sure you need to point to the implementation directly.

  • You're variable for your classes are placed at two different places. I always like to declare all my variables first so I can see when I open the class, I can quickly determine on what my class depends on or what the class is using.

           //filter out malicious stuff here
            if (this.server.processScore(message)) {
    
                try {
                    this.server.saveScores();
                } catch (IOException e) {
                    System.out.println("Saving scores failed");
                }
    
                this.server.sendScores(conn);
            } else {
                //close the connection if the received scores message cannot be parsed
                conn.close(0);
            }                
    
  • I would do it differently. First, I would not use a boolean to determine if the score has been process or not. I would make the assumption that most of the time, the method will work and use exception to manage "garbage". The comment //close the connection if the received scores message cannot be parsed is really redundant.

  • At one point if you think that the High Score is something that a lot of people would use, I would try to cache the result of the list and only modify or recreate the list when something change (you could have a treshold on this) or X number of times in a day. If it would have been a webpage it could have save some request, but since it's sent directly to the client I don't know if you could do something similar. Still, you don't need to recreate the list every time.

  • You should really use the logging provided by your framework. At that point it would remove some unused sysout to something meaningful.

\$\endgroup\$

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.