Multithreaded telnet chat server in Java

Question

I have writed a multithreaded telnet chat application in Java. In main method I run telnet server. In server class at run method I bind in specified port (8189) and I spawn listener threads for every client. In every client listener (MessageHandler class) I print welcome message, prompt user for nickname and I listen for messages in loop. Server.sendAll method propagates message in created telnet chat room at bound port. If user enter "BYE" message, then it parts chat room.

App.java:

package pl.hubot.dev.telnet_chat;

public class App {
    public static void main(String[] args) {
        Server.run();
    }
}

Server.java:

package pl.hubot.dev.telnet_chat;

import java.io.IOException;
//import java.net.InetSocketAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.ArrayList;
import java.util.List;

class Server {
    static void run() {
        try {
            int i = 1;
            ServerSocket serverSocket = new ServerSocket(8189);
            //serverSocket.bind(new InetSocketAddress("192.168.1.2", 8189));

            while (true) {
                Socket incoming = serverSocket.accept();
                System.out.println("Spawning " + i);
                new MessageHandler(incoming);
                i++;
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }

    static void sendAll(String message){
        for (MessageHandler handler : handlers) {
            handler.getOut().println(message);
        }
    }

    static List<MessageHandler> getHandlers() {
        return handlers;
    }

    private static List<MessageHandler> handlers = new ArrayList<MessageHandler>();
}

MessageHandler.java:

package pl.hubot.dev.telnet_chat;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.Scanner;

public class MessageHandler extends Thread {
    MessageHandler(Socket incoming) {
        this.incoming = incoming;
        this.start();
    }

    @Override
    public void run() {
        try {
            try {
                Server.getHandlers().add(this);

                InputStream inputStream = incoming.getInputStream();
                OutputStream outputStream = incoming.getOutputStream();

                Scanner in = new Scanner(inputStream);
                out = new PrintWriter(outputStream, true /* autoFlush */);

                out.println("dev::hubot.pl - Telnet Chat Demo");

                out.println("Choose your nick:");
                String nick = "";
                nick = in.nextLine();

                out.println("Hello! Enter BYE to exit.");

                boolean done = false;
                while (!done && in.hasNextLine()) {
                    String line = in.nextLine();
                    Server.sendAll(nick + ": " + line);
                    if (line.trim().equals("BYE"))
                        done = true;
                }
//          } catch (InterruptedException ex) {

            } finally {
                incoming.close();
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }

    PrintWriter getOut() {
        return out;
    }

    private Socket incoming;
    private PrintWriter out;
}

Answer 1

I scanned the code of Server 3 times, looking for where something is added to handlers. Because that's where I expected it. I thought the code might be broken or that sendAll is an unused method, because handlers seems always empty. Finally I found that it's the MessageHandler instances that use Server.getHandlers(), and append themselves.

Why is MessageHandler aware of the handlers of the server? MessageHandler doesn't really need to be aware of the collection of handlers in the server, does it? It doesn't, and therefore shouldn't. In fact, it would be better if the message handlers were cleanly isolated from each other.

By the same line of reasoning, why is MessageHandler aware of the Server at all? It would be better if it wasn't. If you want to let message handlers interact with the server in some way, it would be better to create a dedicated interface for that, and pass an instance of that interface to message handlers.

It's not recommended to start threads from constructors. It would be better to let Server call .run() on the message handlers right after construction.

Do not nest try blocks within try blocks. Break up the large try-catch to smaller blocks with minimal code within any try. The nesting will naturally disappear, and the code will become easier to understand and to debug.

In Java it's recommended to declare class fields at the top, followed by constructors, followed by methods.