Game Networking between 2 players

I'm making a game and I want to establish a network connection between 2 players through a server.

So far all I have is:

```
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;

public class Server extends Thread {

public ServerSocket s;
public Socket p1, p2;
public PrintWriter bis1, bis2;
public BufferedReader bos1, bos2;

public Server() {
try {
s = new ServerSocket(5_000, 1714);
} catch (IOException e) {
e.printStackTrace();
}
}

public void run() {
try {
p1 = s.accept();
p2 = s.accept();
} catch (IOException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}

try {
bis1 = new PrintWriter(p1.getOutputStream());
bis2 = new PrintWriter(p2.getOutputStream());
bos1 = new BufferedReader(new InputStreamReader(p1.getInputStream()));
bos2 = new BufferedReader(new InputStreamReader(p2.getInputStream()));
} catch (IOException e) {
e.printStackTrace();
}

while (p1.isClosed() || p2.isClosed()) { // if one of the players disconnect, the match will end.
try {
String p1 = bos1.readLine(); // what p1 says
String p2 = bos2.readLine(); // what p2 says

if (!p1.equalsIgnoreCase("")) { // if what p1 says is something
if(p1.startsWith("my position x=")) {
p1 = p1.substring(15);
float x = Float.parseFloat(p1);
bis2.write("his position x=" + x);
} else if(p1.startsWith("my position y=")) {
p1 = p1.substring(15);
float y = Float.parseFloat(p1);
bis2.write("his position y=" + y);
}
}

if (!p2.equalsIgnoreCase("")) { // if what p1 says is something
if(p2.startsWith("my position x

Your vertical separation is good, you've got clear regions in your code for defining the objects you need like sockets, readers and writers, etc. There's one region that needs cleaning, though:

while (p1.isClosed() || p2.isClosed()) { // if one of the players disconnect, the match will end.
    try {
        String p1 = bos1.readLine(); // what p1 says
        String p2 = bos2.readLine(); // what p2 says

        if (!p1.equalsIgnoreCase("")) { // if what p1 says is something
            if(p1.startsWith("my position x=")) {
                p1 = p1.substring(15);
                float x = Float.parseFloat(p1);
                bis2.write("his position x=" + x);
            } else if(p1.startsWith("my position y=")) {
                p1 = p1.substring(15);
                float y = Float.parseFloat(p1);
                bis2.write("his position y=" + y);
            }
        }

        if (!p2.equalsIgnoreCase("")) { // if what p1 says is something
            if(p2.startsWith("my position x=")) {
                p2 = p2.substring(15);
                float x = Float.parseFloat(p2);
                bis1.write("his position x=" + x);
            } else if(p2.startsWith("my position y=")) {
                p2 = p2.substring(15);
                float y = Float.parseFloat(p2);
                bis1.write("his position y=" + y);
            }
        }
    } catch (IOException e) {
        e.printStackTrace();
    }
}

It's filled with duplication. If this game turns into a four-player game, you're gonna have a mess on your hands.

If you could wrap all the processing in a Player object, however, you could extract the relevant parts to functions, drastically simplifying the main loop:

while (!gameEnded(players)) { // check victory conditions or disconnects
    try {
        for(Player p : players){
            handleTickForPlayer(p);//How to handle update x y?
        }
    } catch (IOException e) {
        e.printStackTrace();
    }
}

Okay, that looks rather drastic, but that's because I'm leaving some parts out.

You've combined reading and writing in your own code, and its not very future proof. If you wanted to log matches, you'd have to insert it something in between, and from the way you handle messages, you'd have to do it for every case.

What I think you should do is give your messages some sort of identifier to state the meaning of the message, like id = 0 means it's an update of position x, id = 1 is used for an update for position y, maybe id = 2 is a chat message... you could put the type in the message by doing something like so: "id;message body", so "0;my position x=10.5". Or maybe you'd be better off with well known formats like JSON, but that's a separate point.

Via using these ids, you can then say "well, this is an 'update x' message", and send it off to the "update x" message handler.

I'd propose something like this:

for(Player p : players){
            Message message = readMessageFromPlayer(p);
            if (message == null) { continue; }

            MessageHandler handler = getHandlerForMessage(message);
            if (handler == null) {
                //error?
                //maybe ignore bad messages
                continue;
            }
            handler.handleMessageFromPlayer(message, p, players);
        }

In readMessageFromPlayer, you check if there is any incoming content, and if there is, you split it up into the body of the message and the identifier. Otherwise return null...

In getHandlerForMessage, you check if you have any handler that can handle said message. There's multiple ways to do that; one way is to have a Map of sorts (this means max 1 handler per message type), the other is a List where each MessageHandler has a boolean canHandle(Message message) (multiple possible handlers, but first come first serve), or it could be a gigantic switch that just makes you a new MessageHandler of the correct subclass (nasty, but could work?). I'd go for the Map because it allows you to instantly see "is there a handler for this messagetype".

Then, once you have the handler, you give it the message, the player who sent the message, and the rest of the players in the game.

(Actually, you might want to wrap the game in a Game object, if games are played ON the server. What you have right now is a relay server; the server doesn't keep track of state. That's okay, if all you want is a relay server, but this does stop you from doing things like server-side anti-cheat, such as checking if players are moving into walls.)

Inside the MessageHandler, you'd go about the thing you're doing now; first, parse the message to get the relevant content, then, send out an update.

You might want to change the messages to a more easily parsable format; if you'd go with the idea that the first number; in a message is the id, why not put the rest in the same shape? Something like 0;10.5; being "update x to 10.5", with the format bein

StackExchange Code Review Q#123422, answer score: 7