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<String> names = new HashSet<>();
    private static HashSet<String> userNames = new HashSet<>();
    private static HashSet<PrintWriter> 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("SUBMIT_NAME");
                name = in.readLine();
                serverSideName = name.toLowerCase();

                synchronized (names) {
                    while (names.contains(serverSideName) || name == null || name.trim().isEmpty()) {
                        out.println("RESUBMIT_NAME");
                        name = in.readLine();
                        serverSideName = name.toLowerCase();
                    }
                }

                out.println("NAME_ACCEPTED");
                System.out.println(name + " connected. IP: " + socket.getInetAddress().getHostAddress());

                messageAll("CONNECT" + name);
                userNames.add(name);
                names.add(serverSideName);
                writers.add(out);
                out.println("INFO" + ++usersConnected + names());


                while (true) {
                    String input = in.readLine();

                    if (input == null || input.isEmpty()) {
                        continue;
                    }

                    messageAll("MESSAGE " + name + ": " + input);
                }
            } catch (IOException e) {
                if (name != null) {
                    System.out.println(name + " disconnected.");
                    userNames.remove(name);
                    names.remove(serverSideName);
                    writers.remove(out);
                    messageAll("DISCONNECT" + name);
                    usersConnected--;
                }   
            } finally {     
                try {
                    socket.close();
                } catch (IOException e) {}
            }
        }
    }

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

The Chat Client:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.concurrent.Task;
import javafx.scene.Scene;
import javafx.scene.control.TextArea;
import javafx.scene.control.TextField;
import javafx.scene.control.TextInputDialog;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;


public class ChatClient extends Application {
    final int PORT = 9001;
    final String SERVER_ADDRESS = "localhost";
    BufferedReader in;
    PrintWriter out;

    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) {
        TextArea messageArea = new TextArea();
        messageArea.setEditable(false);

        TextField textField = new TextField();
        textField.setEditable(false);
        textField.setOnAction(e -> {
            out.println(textField.getText());
            textField.clear();
        });

        VBox layout = new VBox(5);
        layout.getChildren().addAll(messageArea, textField);

        stage.setScene(new Scene(layout));
        stage.setTitle("Chatter App By Legato");

        Task task = new Task<Void>() {
            @Override
            public Void call() {
                try {
                    Socket socket = new Socket(SERVER_ADDRESS, PORT);
                    in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
                    out = new PrintWriter(socket.getOutputStream(), true);

                    while (true) {
                        String line = in.readLine();

                        if (line.startsWith("SUBMIT_NAME")) {
                            FutureTask<String> futureTask = new FutureTask<>(new NamePrompt("Choose a screen name:"));
                            Platform.runLater(futureTask);
                            try {
                                out.println(futureTask.get());
                            } catch(InterruptedException | ExecutionException ex) {}
                        } else if (line.startsWith("RESUBMIT_NAME")) {
                            FutureTask<String> futureTask = new FutureTask<>(new NamePrompt("Duplicate name. Try another:"));
                            Platform.runLater(futureTask);
                            try {
                                out.println(futureTask.get());
                            } catch(InterruptedException | ExecutionException ex) {}
                        } else if (line.startsWith("NAME_ACCEPTED")) {
                            textField.setEditable(true);
                            Platform.runLater(() -> textField.requestFocus());
                        } else if (line.startsWith("INFO")) {
                            messageArea.appendText("Users connected: " + line.charAt(4) + '\n' + line.substring(5) + "\n\n");
                        } else if (line.startsWith("CONNECT")) {
                            messageArea.appendText(line.substring(7) + " has connected.\n\n");
                        } else if (line.startsWith("MESSAGE")) {
                            messageArea.appendText(line.substring(8) + '\n');
                        } else if (line.startsWith("DISCONNECT")) {
                            messageArea.appendText(line.substring(10) + " has disconnected.\n\n");
                        }
                    } 
                } catch(IOException ioe) {
                    messageArea.appendText("Server is offline.\nPlease exit.");
                }

                return null;
            }
        };

        Thread severIO = new Thread(task);
        severIO.setDaemon(true);
        severIO.start();

        stage.show();
    }

    class NamePrompt implements Callable<String> {
        String message;

        NamePrompt(String message) {
            this.message = message;
        }

        @Override
        public String call() {
            TextInputDialog dialog = new TextInputDialog();
            dialog.setTitle("Welcome to Chatter");
            dialog.setHeaderText("Screen name selection");
            dialog.setContentText(message);
            dialog.setGraphic(null);
            return dialog.showAndWait().get();
        }
    }
}

I'd like to focus on:

  1. General critique on my thread handling.
  2. The way I handle the client and client input.
  3. I've also wondering about using flush for the streams. It doesn't seem to make a difference but would it increase performance, especially when this is over the net? I've only tested locally, so far.
  4. The Server and Server protocol.
  5. The overall performance and efficiency.
  6. The overall extensibility of my design choices.

Any feedback touching on best practices, superior library or library use, or general readability of the program is welcomed. I haven't handled some of the exceptions yet, that is intentional while I was creating this.

For anyone interested this is the Github repository.

up vote 12 down vote accepted

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

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 ",".

  • 4
    The good thing about implementing Runnable is when you eventually use ThreadPool and other more "sophiscated" way of handling thread, you won't have to change anything, since they "consume" Runnable. – Marc-Andre Sep 15 '15 at 14:31
  • 3
    @Marc-Andre - While I agree that people should implement runnable, instead of extending thread, that specific reason is not valid - Threads themselves implement Runnable, so can be subnmitted to a service ;-) The real issue is that threads run stuff, and what your code does implements what's run, not what's running it, so the OOP is wrong if your extend Thread, because your object is not a Thread, but something run on a thread. – rolfl Sep 19 '15 at 11:56

You can have more classes

Your start(Stage stage) is doing a bit too much for what I like to see. You're creating an Anonymous Class that encompass the logic of reading the socket and the presentation of to the client. I would create a separate class that would extract that logic. This would need to pass the socket and the textbox, but I'm sure there is an elegant to do that, either via the constructor or having a class that would encapsulate that part of the logic.

Enum is your friend

At the moment you use directly the String of your command in your code. This is fine since you only have two classes, but the chance for errors is there. I would create an Enum that would look like this :

public enum Command {

    SUBMIT_NAME("SUBMIT_NAME"),
    RESUBMIT_NAME("RESUBMIT_NAME"),
    NAME_ACCEPTED("NAME_ACCEPTED"),
    INFO("INFO"),
    CONNECT("CONNECT"),
    MESSAGE("MESSAGE"),
    DISCONNECT("DISCONNECT");

    private Command(final String pValue){
        value = pValue;
    }

    private String value;

    public String getValue() {
        return value;
    }
}

Now you have only one source and you can't mistype a command (well you still can, but at least you'll have the typo on both sides).

Runnable

Like @rolfl said in his comment, the good reason to not inherit Thread is that you're not modifying/extending the functionality of the Thread class. You are implementing code that need to run on a Thread which is the responsibility of the Runnable interface. (Both of his comment are spot on, so I recommend you read both of them.)

I still don't know much about threading, but at some point you will want to use ThreadPool or some other threading classes that will manage thread (this is the hard part in threading) and you'll only have to pass those Runnable.

Don't leak implementation

You're using HashSet<> directly instead of using Set<>. This is the same thing as with List and pretty much any interface. Unless you really need that particular implementation for a specific reason, use the interface. Your code will then be free of being locked with a specific implementation.

up vote 10 down vote
+100

Server

  • I think you are over-using the static keyword. Don't make each and every function static, create an object of the class and use the object instead.

  • You have some catch parts that are either empty or print very little information. I suggest you investigate a bit more about the different reasons those exceptions can occur and add some comments about what cases you want to ignore.

  • Use composition over inheritance, do not extend Thread. Instead implement Runnable as suggested by others.

  • That's one big start method in ClientHandler. You should cut it down to separate parts, such as: requestName, connected, messageLoop.

  • If you use a ConcurrentHashSet for the names you don't need to use synchronized (names).

  • Overall it seems like you are synchronizing too little, which can lead to concurrency bugs. You are not synchronizing on anything when you add/remove in userNames, names and writers.

  • I don't understand why you have the usersConnected variable (which is also not part of any synchronization btw). Why not use the size of one of the sets instead of this int variable?

  • userNames, names, and writers are very bound together. I would refactor this to a Set<Client> where a client has a userName, name and writer. This set could preferably be a ConcurrentHashSet.

Client

  • Please, at least make your class fields private.

  • You can use Java FXML files to specify the layout instead of creating the layout programmatically.

  • Both the client and the server are relying on the default encoding, with for example InputStreamReader. Consider using UTF-8 instead. I believe that if you would use the findbugs tool this would also alert you about this.

FutureTask

This part is duplicated code:

FutureTask<String> futureTask = new FutureTask<>(new NamePrompt("Choose a screen name:"));
Platform.runLater(futureTask);
try {
    out.println(futureTask.get());
} catch(InterruptedException | ExecutionException ex) {}

You could easily refactor that to a createNamePromptTask("Choose a screen name:")

Also, your FutureTask is calling showAndWait on the dialog, making it a synchronous dialog. I'd prefer it if the dialog was created, shown, and the code continued. Then, when the dialog was closed, you use an event listener or callback which performs out.println(/* get value of text field in dialog */); You might be able to accomplish this by using one of the onXYZ properties in Dialog.

Server is offline

messageArea.appendText("Server is offline.\nPlease exit.");

Are you sure that the only reason this can happen is because the server is offline? What if you disconnect your network cable?

Also, considering the number of times you call messageArea.appendText, you might want to extract a method message(String) or similar.

Message types

It seems like once the client is connected, it can only send one message type to the server, a chat message. I recommend that you open up the possibility to send other message types as well by appending a string like "MESSAGE" to the front of the current message that is sent from client to server.

All your else-ifs essentially works as a Map<String, Consumer<String>>, that is: You check what string the message starts with, and then you handle it.

You could refactor this code to use a real Map<String, Consumer<String>>, which could lead to cleaner code and being a bit easier to handle the messages. There's only one small down-side (which I don't think of as a downside, really): Your category strings ("MESSAGE", "RESUBMIT_NAME" etc) would have to be of the same length, or you could use a space character as a delimiter between the message type and the message data.

Consider this:

Map<String, Consumer<String>> handlers = new HashMap<>();
handlers.put("NAME", this::submitName);
handlers.put("INFO", str -> messageArea.appendText("Users connected: " + str.charAt(0) + '\n' + str.substring(1) + "\n\n"));
handlers.put("CONN", str -> messageArea.appendText(str + " has connected.\n\n"));
...

Notice that here I changed so that str is always without the message type. Your current code has lines like:

messageArea.appendText(line.substring(8) + '\n');

Guess what 8 is? A magic number, yes!

Instead you could let one piece of code take care of the splitting of message type and message data:

while (true) {
    String line = in.readLine();
    int splitter = line.indexOf(' ');
    String type;
    String data;
    if (splitter == -1) {
        // only a message type was received
        type = line;
        data = "";
    } else {
        type = line.substring(0, splitter);
        data = line.substring(splitter);
    }
    Consumer<String> handler = handlers.get(type);
    handler.accept(data);

Your specific questions

  1. Thread handling: It would be a good practice to check if the thread has been interrupted, by doing something like if (Thread.interrupted()) break; when you are inside a loop. Otherwise, it looks OK.

  2. Client and client input: I'm not a fan of the showAndWait which leads to a synchronous dialog. Your three tightly-related variables in Server userNames, names, writers is also a smell to me.

  3. Flushing the stream: You don't need to call it explicitly because you construct the PrintWriter with the parameter true.

  4. Server protocol: The INFO message has a flaw, that it would show incorrectly if there are 10 or more users connected. Additionally, personally I prefer the "categories" (INFO, SUBMIT_NAME, etc) to be of the same length, as I wrote above.

  5. Performance and efficiency should be fine.

  6. Extensibility of design choices? Most importantly: Avoid static.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.