Game server packet handler

I am developing an online game and my code is getting pretty hard to work with. I would appreciate any suggestions how to clean it up or make it simpler to work with. Thanks for any suggestions.

```
import java.io.BufferedWriter;
import java.io.FileWriter;
import java.io.IOException;
import java.util.ArrayList;
import server.engine.*;
import java.util.Arrays;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.logging.Level;
import java.util.logging.Logger;

public class ServerPacketHandler extends PacketHandler {

public static CopyOnWriteArrayList usersOnline = new CopyOnWriteArrayList();

@Override
public void handlePacket(Packet p) throws ArrayIndexOutOfBoundsException {
ArrayList newcontent = new ArrayList();
for (String c : p.content) {
newcontent.add(c.replace("", "").replace("", ""));
}
p.content = newcontent.toArray(new String[newcontent.size()]);
if(Server.debugMode) CustomLog.info(p.id + " " + Arrays.toString(p.content));
if (!p.user.isLogged) {
switch (p.id) {
case 0: { //user sent login information
if (!p.content[2].equals(Server.version)) {
p.user.sendPacket(-2);
return;
}
User user;
if ((user = UserManager.loadUser(p.content[0])) != null) {
if (!p.content[1].equals(user.password)) {
p.user.sendPacket(-1, "Incorrect login");
return;
}
p.user.isLogged = true;
p.user.id = p.content[0];
p.user.password = p.content[1];
if (user.username.equals("null")) {
p.user.sendPacket(2);
} else {
for (User u : usersOnline) {

my code is getting pretty hard to work with.

That's usually a sign that your code smells (excuse the expression).

Despite having 'Hello, World!' as my Java experience, there's quite a bit I can comment on.

Overall Comments:

• Your code is aiming to do everything, all bundled together.

You should be spreading it out a bit more.

Stop going for a God object, and settle with a President object (That's not a real thing, I was trying to be witty).

• Why are you returning HTML in your packets ...?

HTML is for marking up pages with specific styling (`,
), not optimised for returning data.

Consider using JSON instead, as it is designed for returning data in the form of an object.

By converting the JSON into a Java array, you can access all the properties without having to split by delimiters.

Also among the pros is that it's widely implemented across other systems, too.

HTML takes way more bytes to send data with delimiters also, meaning the response time will be slower.

• Why are you sending packets by magic numbers (unexplained numbers)?

First, you should define those numbers as constants.

Actually, that would be the thing to do. Except, using integers like that is bad practice. It's easily confusable, and unnecessarily complicated. Use strings instead of integers there.

And instead of a switch, use an object, and associate variables to the keys (strings, not integers).

c.replace("", "").replace("", "")

Stacking .replaces is bad practice, use an array instead.

switch (topic) {
    case "General":
        match.topics.add(Question.Topic.General);
        break;
    case "Technology":
        match.topics.add(Question.Topic.Technology);
        break;
    case "History":
        match.topics.add(Question.Topic.History);
        break;
    case "Geography":
        match.topics.add(Question.Topic.Geography);
        break;
    case "Sport":
        match.topics.add(Question.Topic.Sport);
        break;
    case "Custom":
        match.topics.add(Question.Topic.Custom);
        break;
    default:
        p.user.sendPacket(-3);
        return;

There's better ways to do this, than to use a switch.

Consider an object, or something better there.

You write this more than once, also. Consider converting this into a function.

if (!password.equals("[null]")) { 
    match.password = password;
} else match.password = "";

First, please, please wrap your statements in brackets, bad things can happen if you mess that up.

Consider a ternary statement instead. Ternary statements let you simplify small if-else statements, and even provide job security (joke).

For example, here, using a ternary statement, you would write:

match.password = password.equals("[null]") ? "" : password;

A ternary here too:

if (list.size() > 0) {
    return list;
} else {
    return null;
}

into:

return list.size() > 0 ? list : null;

if (p.content[0].equals("1")) {
    p.user.match.usersAnswer1.add(p.user);
}
if (p.content[0].equals("2")) {
    p.user.match.usersAnswer2.add(p.user);
}
if (p.content[0].equals("3")) {
    p.user.match.usersAnswer3.add(p.user);
}
if (p.content[0].equals("4")) {
    p.user.match.usersAnswer4.add(p.user);
}

There's a better ways to approach this: switch (which you seem confident with) or array (not an object, as the keys would be integers anyway).

if (!found){
    // Lots of code here.
} else {
    p.user.sendPacket(-3);
}

You should be flipping things like this, so that the smaller one comes first. Especially seeing as it's a false test.

String answer1 = p.content[1];
String answer2 = p.content[2];
String answer3 = p.content[3];
String answer4 = p.content[4];
String answerNumber = p.content[5];

• What would happen if you wanted a 25 answer quiz? You'd have to have it all the way up to answer25.

• Don't manually type out variables like that.

Use a loop (for or foreach, whatever floats your boat) instead.

Speaking of bad practice, this should really be improved:

writer.write(question + ";;" + answer1 + ";;" + answer2 + ";;" + answer3 + ";;" + answer4 + ";;" + answerNumber + ";;" + topic + System.getProperty("line.separator"));

Fortunately, there's an easier way to do this. Use an array. By using an array, you can just join all the variables together with some glue (a string, in this case: ";;")

String[] items = {question, answer1, answer2, answer3, answer4, answerNumber, topic};
String combinedString = StringUtils.join(items, ";;");
combinedString += System.getProperty("line.separator");
writer.write(combinedString);

Although, using a loop here would be much better, it'd probably have a similar style (each answer being added to array, that would be joined after all the elements have been iterated over).

See here for more info on array joining and the StringUtils class.

Or, as @Vogel612 kindly pointed out; you can use Collections.Joining instead:

``
String combinedSt

StackExchange Code Review Q#101166, answer score: 10