Java Multi-threaded Chat Room

Question

In an attempt to learn more with Java, I decided to challenge myself by creating a GUI-based, multi-threaded chat room. Currently, you run the Client and it prompts you to either create or join a server based on an IP. Port used is 9000. Any suggestions on how I can improve?

Server.java:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.sql.Timestamp;
import java.util.HashSet;
import java.util.Set;

public class Server extends Thread {
    /**
     * Used to write to all the connected clients.
     */
    private static final Set<PrintWriter> WRITERS = new HashSet<>();

    /**
     * Used to keep track of all of th enames of connected clients.
     */
    private static final Set<String> NAMES = new HashSet<>();

    /**
     * Used to keep track of the current client's ID for logging purposes.
     */
    private static int clientId = 1;

    /**
     * Used when creating a server.
     */
    private final ServerSocket listener;

    public Server(ServerSocket listener) {
        this.listener = listener;
    }

    /**
     * Used to log messages.
     * 
     * @param message
     */
    public static void log(String message) {
        String timestamp = new Timestamp(System.currentTimeMillis()).toString();
        timestamp = timestamp.substring(0, timestamp.length() - 4);
        System.out.println("[" + timestamp + "] " + message);
    }

    /**
     * Returns the ServerSocket listener.
     * 
     * @return
     */
    public ServerSocket getListener() {
        return this.listener;
    }

    @Override
    public void run() {
        if (listener != null) {
            try {
                if (listener.isBound()) {
                    log("Server started on " + (listener.getInetAddress().getHostAddress()) + " and listening on port "
                            + ServerConstants.PORT);
                }
                while (true) {
                    new User(listener.accept(), clientId++).start();
                }
            } catch (IOException e) {
                e.printStackTrace();
            } finally {
                try {
                    listener.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }
    }

    /**
     * Creates a server and starts accepting inputs.
     * 
     * @param port
     * @throws IOException
     */
    public static Server createServer(int port) throws IOException {
        return new Server(new ServerSocket(port));
    }

    /**
     * This class listens for and sends messages to messages from all clients.
     * 
     * @author ivisc
     *
     */
    private static class User extends Thread {
        private Socket socket;
        private PrintWriter out;
        private BufferedReader in;
        private String address;
        private String name;
        private int id;

        public User(Socket sock, int id) {
            this.socket = sock;
            this.id = id;
            this.address = socket.getInetAddress().getHostAddress();
        }

        @Override
        public void run() {
            try {
                log("Starting connection from " + this.toString());
                // Start up our variables for use with the socket.
                out = new PrintWriter(socket.getOutputStream(), true);
                in = new BufferedReader(new InputStreamReader(socket.getInputStream()));

                // Ask for a name
                while (true) {
                    out.println("ENTERNAME");
                    String line = in.readLine();
                    if (line == null) {
                        return;
                    }
                    if (!Filter.filter(line)) {
                        out.println("INVALIDNAME");
                    } else {
                        synchronized (NAMES) {
                            if (!NAMES.contains(line)) {
                                NAMES.add(line);
                                name = line;
                                out.println("NAMEACC");
                                break;
                            }
                        }
                    }
                }
                WRITERS.add(out);

                log("Registering " + this.toString() + " (" + name + ")");
                for (PrintWriter writer : WRITERS) {
                    writer.println("NEWMEM" + name);
                }
                // Now we listen for names
                while (true) {
                    String message = in.readLine();
                    if (message == null) {
                        return;
                    }
                    if (!Filter.filter(message)) {
                        out.println("INVALIDMESSAGE");
                    } else {
                        for (PrintWriter writer : WRITERS) {
                            writer.println("MESSAGE" + name + ": " + message);
                        }
                    }
                }
            } catch (IOException e) {
                log(this.toString() + " is disconnecting...");
            } finally {
                log("Shutting down I/O from " + this.toString() + " (" + id + ")");
                try {
                    out.close();
                    in.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }

        @Override
        public String toString() {
            return address + "(" + id + ")";
        }
    }
}

Client.java:

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.Scanner;

import javax.swing.JFrame;
import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.JTextField;

@SuppressWarnings("serial")
public class Client extends JFrame {
    private JTextField textField;
    private JTextArea textArea;
    private Socket socket;
    private PrintWriter out;
    private BufferedReader in;

    public Client() {
        textField = new JTextField(40);
        getContentPane().add(textField, BorderLayout.SOUTH);
        textField.setEditable(false);

        textArea = new JTextArea();
        getContentPane().add(textArea, BorderLayout.NORTH);
        textArea.setEditable(false);
        getContentPane().add(new JScrollPane(textArea), BorderLayout.CENTER);

        textField.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent arg0) {
                out.println(textField.getText());
                textField.setText("");
            }
        });

        setPreferredSize(new Dimension(800, 600));
        pack();
        setSize(getPreferredSize());
        setLocationRelativeTo(null);
        setVisible(true);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    public void initVars() throws IOException {
        this.out = new PrintWriter(socket.getOutputStream(), true);
        this.in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
    }

    public void connect(String address) {
        try {
            this.socket = new Socket(address, ServerConstants.PORT);
            initVars();
            this.textField.setEditable(true);
            Server.log("Connected to " + address);
            this.setVisible(true);
            while (true) {
                new ServerListener(out, in).start();
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public void connect(Server server) {
        try {
            String address = server.getListener().getInetAddress().getHostAddress();
            this.socket = new Socket(address, ServerConstants.PORT);
            initVars();
            this.textField.setEditable(true);
            Server.log("Connected to " + address);
            this.setVisible(true);
            while (true) {
                new ServerListener(out, in).start();
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        Object[] options = new Object[] { "Start", "Join", "Cancel" };
        int input = JOptionPane.showOptionDialog(null, "Would you like to create or join a server?",
                "Chat Room Selection", JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.INFORMATION_MESSAGE, null, options,
                options[1]);
        Client client = new Client();
        switch (input) {
            case 0:
                try {
                    Server server = Server.createServer(ServerConstants.PORT);
                    server.start();
                    client.connect(server);
                } catch (IOException e) {
                    e.printStackTrace();
                }
                break;
            case 1:
                String address = JOptionPane.showInputDialog("Please enter server IP");
                if (address != null) {
                    client.connect(address);
                }
                break;
            case 2:
                System.exit(0);
                break;
        }
        s.close();
    }

    private class ServerListener extends Thread {
        private PrintWriter out;
        private BufferedReader in;

        public ServerListener(PrintWriter out, BufferedReader in) {
            this.out = out;
            this.in = in;
        }

        @Override
        public void run() {
            while (true) {
                try {
                    handle(in.readLine());
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }

        public void handle(String message) throws IOException {
            System.out.println("handling " + message);
            if (message == null) {
                return;
            }
            System.out.println(message);
            if (message.startsWith("MESSAGE")) {
                message = message.substring(7);
                textArea.append(message + "\n");
            } else if (message.startsWith("ENTERNAME")) {
                while (true) {
                    String name = JOptionPane.showInputDialog("Please enter a username");
                    out.println(name);
                    String response = in.readLine();
                    if (response.equals("INVALIDNAME")) {
                        JOptionPane.showMessageDialog(null, "Name does not pass Name Filter", "Invalid Name",
                                JOptionPane.ERROR_MESSAGE);
                    } else {
                        break;
                    }
                }
            } else if (message.startsWith("NEWMEM")) {
                String name = message.substring(6);
                textArea.append(name + " has joined the server.\n");
            }
        }
    }
}

There are a few references to ServerConstants.java and Filter.java here, but I won't include them for brevity's sake. Basically, ServerConstants is an abstract interface that just defines the port to use, and Filter is a simple name filter using a list of "bad words" (I believe it's this one that I used).

I know I didn't comment everything, and adding throws statements might not be best practice, and extending from JFrame is usually not recommended, but, based on everything here, any suggestions?

Answer 1

First of all, it's nice to see at least some comments. It makes understanding your code a lot easier.

To the suggestions:
Yes, you are right, inheriting from JFrame isn't good. Inheriting from Thread isn't good, too. Your case is a little bit worse because your code says, your client is a JFrame, your server is a thread and your user is a thread, too. That's really confusing. The problem is, that you are mixing your domain logic with the technical stuff.
Ask yourself this: What do you want your program to do (from a users standpoint)? I would say you want to have users that can send each other messages. That's something I would assume to find in your User class.
The GUI and the threads are something completely different. They are on a different layer and your user class shouldn't have any knowledge about them.

Additionally, your classes are doing too much and it's not easy to predict who will do what. You should ask yourself before you're creating a method: What's its job? What will be the output of this method and how would I test it?

Take this for example:

    public void connect(String address) {
        try {
            this.socket = new Socket(address, ServerConstants.PORT);
            initVars();
            this.textField.setEditable(true);
            Server.log("Connected to " + address);
            this.setVisible(true);
            while (true) {
                new ServerListener(out, in).start();
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

This method would be really hard to test because it does multiple, very different things and because you have two hard dependencies: Socket and ServerListener. If you don't have an internet connection, your test would fail. If your ServerListener fails, your test fails, too. Try to get rid of such dependencies and let them take interfaces instead of classes. This way you could implement an alternative version of the class to use it in the tests.

Besides, your two connect classes look almost identical. You should create a method for overlapping logic.

And finally:

    while (true) {
        new ServerListener(out, in).start();
    }

You are creating an object in an infinite loop and I guess you will leave the loop by throwing and catching an exception, right? Don't do that. Exceptions are for exceptional cases and not to control the flow. Use ifs instead.