patternjavaModerate

Network chat app

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

chatnetworkapp

Problem

Applying a lot of unprecedented concepts here, it's a simple chat server capable of handling multiple clients, which are run through JavaFX, and have their individual threads instantiated and handled within the application thread (was more complex than anticipated).

The Chat Server:

```
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Date;
import java.util.HashSet;
import java.util.Scanner;

public class ChatServer {
private static final int PORT = 9001;
private static HashSet names = new HashSet<>();
private static HashSet userNames = new HashSet<>();
private static HashSet writers = new HashSet<>();
private static int usersConnected = 0;

public static void main(String[] args) {
System.out.println(new Date() + "\nChat Server online.\n");

try (ServerSocket chatServer = new ServerSocket(PORT)) {
while (true) {
Socket socket = chatServer.accept();
new ClientHandler(socket).start();
}
} catch (IOException ioe) {}
}

private static String names() {
StringBuilder nameList = new StringBuilder();

for (String name : userNames) {
nameList.append(", ").append(name);
}

return "In lobby: " + nameList.substring(2);
}

private static class ClientHandler extends Thread {
private String name;
private String serverSideName;
private Socket socket;
private BufferedReader in;
private PrintWriter out;

public ClientHandler(Socket socket) {
this.socket = socket;
}

@Override
public void run() {
try {
in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
out = new PrintWriter(socket.getOutputStream(), true);

out.println

Solution

Server

private static String names() {
    StringBuilder nameList = new StringBuilder();

    for (String name : userNames) {
        nameList.append(", ").append(name);
    }

    return "In lobby: " + nameList.substring(2);
}

The above can be simplified by streaming and collecting with Collectors.joining(CharSequence, CharSequence, CharSequence) using String.join(CharSequence, Iterable) (thanks @toto2!):

private static String names() {
    // return userNames.stream().collect(Collectors.joining(", ", "In lobby: ", ""));
    return "In lobby: " + String.join(", ", userNames);
}

Instead of extends Thread for ClientHandler, maybe you can also consider implements Runnable as well... Of course, you will have to replace new ClientHandler(socket).start() with new Thread(new ClientHandler(socket)).start() too.

private static void messageAll(String... messages) {
    if (!writers.isEmpty()){
        for (String message : messages) {
            for (PrintWriter writer : writers) {
                writer.println(message);
            }
        }
    }
}

I don't think you stand to gain much optimization by checking if you have any writers in the first place... It's a nice-to-have thing, but I will rather do an early return instead of nesting code blocks. A stream-based way of doing the same might be:

private static void messageAll(String... messages) {
    writers.forEach(w -> Stream.of(messages).forEach(w::println));
}

The other benefit here is that your isEmpty() check also becomes internalized, and therefore redundant for you to do so explicitly.

Client

Not much to comment on here, except that maybe you can consider replacing the String-based comparisons in your call() method with an enum-driven approach, such as (please consider how the variables like out and messageArea can be accessed correctly too, I did not factor that in):

enum Action implements Consumer {
    SUBMIT_NAME {
        @Override
        public void accept(String input) {
            FutureTask futureTask = new FutureTask<>(
                                                new NamePrompt("Choose a screen name:"));
            Platform.runLater(futureTask);
            try {
                out.println(futureTask.get());
            } catch(InterruptedException | ExecutionException ex) {}
        }
    }, 
    // ...
    INFO {
        @Override
        public void accept(String input) {
            int delimiter = input.indexOf(',');
            messageArea.appendText(String.format("Users connected: %s%n%s%n%n", 
                    input.substring(0, delimiter), input.substring(delimiter + 1)));
        }
    }, 
    // ...
    DISCONNECT {
        @Override
        public void accept(String input) {
            messageArea.appendText(input + " has disconnected.\n\n");
        }
    };

    static void handle(String input) {
        EnumSet.allOf(Action.class).stream()
                .filter(v -> input.startsWith(v.toString()))
                .forEach(v -> v.accept(input.substring(v.toString().length())));
    }
}

The handling is done entirely within the handle(String) method, which loops through the toString() representation of each enum equally well, while taking care of pre-formatting the actual input to 'consume' by doing input.substring(v.toString().length()). Just one thing to take note of for the "INFO" handling: it looks like you are hard-coding the number of clients to be a single-digit, so I have taken the liberty of illustrating how you can work around that by using a strategically-placed delimiter, like ",".

Code Snippets

private static String names() {
    StringBuilder nameList = new StringBuilder();

    for (String name : userNames) {
        nameList.append(", ").append(name);
    }

    return "In lobby: " + nameList.substring(2);
}

private static String names() {
    // return userNames.stream().collect(Collectors.joining(", ", "In lobby: ", ""));
    return "In lobby: " + String.join(", ", userNames);
}

private static void messageAll(String... messages) {
    if (!writers.isEmpty()){
        for (String message : messages) {
            for (PrintWriter writer : writers) {
                writer.println(message);
            }
        }
    }
}

private static void messageAll(String... messages) {
    writers.forEach(w -> Stream.of(messages).forEach(w::println));
}

enum Action implements Consumer<String> {
    SUBMIT_NAME {
        @Override
        public void accept(String input) {
            FutureTask<String> futureTask = new FutureTask<>(
                                                new NamePrompt("Choose a screen name:"));
            Platform.runLater(futureTask);
            try {
                out.println(futureTask.get());
            } catch(InterruptedException | ExecutionException ex) {}
        }
    }, 
    // ...
    INFO {
        @Override
        public void accept(String input) {
            int delimiter = input.indexOf(',');
            messageArea.appendText(String.format("Users connected: %s%n%s%n%n", 
                    input.substring(0, delimiter), input.substring(delimiter + 1)));
        }
    }, 
    // ...
    DISCONNECT {
        @Override
        public void accept(String input) {
            messageArea.appendText(input + " has disconnected.\n\n");
        }
    };

    static void handle(String input) {
        EnumSet.allOf(Action.class).stream()
                .filter(v -> input.startsWith(v.toString()))
                .forEach(v -> v.accept(input.substring(v.toString().length())));
    }
}

Context

StackExchange Code Review Q#102020, answer score: 12

Revisions (0)

No revisions yet.