Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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

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.

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.

"HELLO" should be sent by client thread
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478
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.

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());
 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.

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.

Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

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());
 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.

lang-java

AltStyle によって変換されたページ (->オリジナル) /