Controller for a MUD client

Question

I want to ensure that, as much as possible given my skill level, that I'm using relevant patterns correctly and following naming conventions. This is a controller for a poor-mans MUD client using Apache TelnetClient. The project is on GitHub.

The startReadPrintThreads starts threads by instantiating workers, which in turn start the I/O threads. When a message is received, the CharacterDataQueueWorker notifies the controller, and the controller will then sendMessages from the ConcurrentLinkedQueue<Command>, to which both i/o threads can add. Only sendCommands can pop, or remove, a Command from the commandsQueue.

1. Is this producer-consumer, or a backwards producer consumer pattern?
2. Are the naming conventions followed?
3. Are there any glaring mistakes or antipatterns?

package telnet;

import static java.lang.System.out;
import java.io.IOException;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.SocketException;
import java.net.UnknownHostException;
import java.util.Iterator;
import java.util.Observable;
import java.util.Observer;
import java.util.Properties;
import java.util.concurrent.ConcurrentLinkedQueue;
import org.apache.commons.net.telnet.TelnetClient;

public final class Controller implements Runnable, Observer {

    private TelnetClient telnetClient = new TelnetClient();
    private InputStreamWorker remoteOutputWorker = new InputStreamWorker();
    private ConsoleReader localInputReader = new ConsoleReader();
    private CharacterDataQueueWorker remoteDataQueueWorker = new CharacterDataQueueWorker();
    private RemoteOutputRegexMessageWorker remoteMessageWorker = new RemoteOutputRegexMessageWorker();
    private final ConcurrentLinkedQueue<Character> remoteCharDataQueue = new ConcurrentLinkedQueue();
    private final ConcurrentLinkedQueue<Command> commandsQueue = new ConcurrentLinkedQueue();

    private Controller() {
    }

    public void startReadPrintThreads() throws SocketException, IOException {
        remoteOutputWorker.print(telnetClient.getInputStream(), remoteCharDataQueue);
        localInputReader.read();
        localInputReader.addObserver(this);
        remoteDataQueueWorker.read(remoteCharDataQueue);
        remoteDataQueueWorker.addObserver(this);
    }

    private void sendCommands() {
        String commandString = null;
        Iterator it = commandsQueue.iterator();
        byte[] commandBytes = null;
        OutputStream outputStream = telnetClient.getOutputStream();
        while (it.hasNext()) {
            try {
                commandBytes = commandsQueue.remove().getCommand().getBytes();
                outputStream.write(commandBytes);
                outputStream.write(10);
                outputStream.flush();
            } catch (IOException ex) {
                out.println("sendCommand\n" + ex);
            }
        }
    }

    @Override
    public void update(Observable o, Object arg) {

        if (o instanceof CharacterDataQueueWorker) {
            String remoteOutputMessage = remoteDataQueueWorker.getFinalData();
            remoteMessageWorker.parseWithRegex(remoteOutputMessage);
            sendCommands();
        }

        if (o instanceof ConsoleReader) {
            String commandString = localInputReader.getCommand();
            Command command = new Command(commandString);
            commandsQueue.add(command);
            sendCommands();
        }
    }

    @Override
    public void run() {
        try {
            Properties props = PropertiesReader.getProps();
            InetAddress host = InetAddress.getByName(props.getProperty("host"));
            int port = Integer.parseInt(props.getProperty("port"));
            telnetClient.connect(host, port);
            startReadPrintThreads();
        } catch (UnknownHostException ex) {
            out.println(ex);
        } catch (SocketException ex) {
            out.println(ex);
        } catch (IOException ex) {
            out.println(ex);
        }
    }

    public static void main(String[] args) throws SocketException, IOException {
        new Controller().run();
    }
}

Answer 1

Member Variables: You define two of them as final, yet never a assign a new value to any of them. Might as well just make them all final.

---

sendCommands():

• commandString is never being used.
• commandBytes define it within the tightest scope as you can. If you define it as the first line of the try, you never have to worry about it being null or having some other value from a previous usage.
• It seems weird that you are using it.hasNext() but never calling it.next() to do the iteration. If all you care about is if the collection has any remaining values, use !commandsQueue.isEmpty(). What you are doing is depending on an implementation detail that might be changed. The returned iterator could be of a snapshot of the collection and not change if the collection is later altered.

---

update():

I prefer to have 2 Observer instances instead of having to check the type of the Observable passed into the method.