Return to Revisions

Your exception handling for new ServerSocket(port) could be better. I don't like the way System.exit(1) is buried in a helper function. The exception should be caught in such a way that it bypasses the clientSocket-accepting loop.

Similarly, if serverSocket.accept() throws an exception, you log it, but blithely proceed to try to create a client thread anyway. Instead, the client-thread-creating code should be inside the try block, so that the exception would cause it to be skipped.

Here is a preliminary rearrangement to fix the exception handling.

public void run() { 
 try {
 serverSocket = new ServerSocket(port);
 isRunning = true;
 while (isRunning) {
 try {
 Socket clientSocket = serverSocket.accept();
 Client client = new Client(clientSocket, this);
 connectedClients.put(client.getID(), client);
 Thread clientThread = new Thread(client);
 clientThread.start();
 logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
 // The "hello" should be sent by the clientThread code instead
 // client.send("@SERVER:HELLO");
 } catch (IOException e) {
 logEvent("Could not accept incoming connection.");
 }
 }
 } catch (IOException | IllegalArgumentException e) {
 System.out.println("Could not bind socket to given port number.");
 System.out.println(e);
 System.exit(1);
 }
}

If you would like to offload some responsibility from this method, you could create "client tasks" to be put into a "client processing pool". Then this run() method could be wonderfully generic.

As noted above, client.send("@SERVER:HELLO") belongs in the clientThread code, not in this loop.