# Controller for a MUD client

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.SocketException;
import java.net.UnknownHostException;
import java.util.Iterator;
import java.util.Observable;
import java.util.Observer;
import java.util.Properties;
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 CharacterDataQueueWorker remoteDataQueueWorker = new CharacterDataQueueWorker();
private RemoteOutputRegexMessageWorker remoteMessageWorker = new RemoteOutputRegexMessageWorker();

private Controller() {
}

remoteOutputWorker.print(telnetClient.getInputStream(), remoteCharDataQueue);
}

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

Command command = new Command(commandString);
sendCommands();
}
}

@Override
public void run() {
try {
int port = Integer.parseInt(props.getProperty("port"));
telnetClient.connect(host, port);
} 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();
}
}

• By the way... Don't you think, that a comment "This question came from our site for professional and enthusiast programmers" visible, when a question has been moved from SO, suggests, that this forum is dedicated for new, unskilled programmers with no enthusiasm ? :( The first one is probably correct, but what with the second one... : ) – guitar_freak Sep 8 '13 at 7:20
• LOL, yeah, maybe that verbiage allows that inference. OTOH, what's the message going the other way? – Thufir Sep 8 '13 at 7:44

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.

• thanks, in particular, for help with the iterator. Yes, I just want to pop commands until it's empty. Why does a System.out from finally, in the iteration of commandQueue, keep outputting when running. Is there an uncaught exception? – Thufir Sep 4 '13 at 19:15
• @Thufir: It's hard to tell what you are asking. However, it sounds like you are asking "Why doesn't this code work correctly in this specific instance?" your initial question was moved to Code review because it was asking "does my code look good?" This follow up question seems like it would be a better fit for StackOverflow if it was fleshed out with more code, information about the inputs, expected outputs and what the actual output is. – unholysampler Sep 4 '13 at 20:45
• The original question could be boiled down to "am I doing this wrong?" in a general sense. So that I didn't go too far with bad design choices. The bug mentioned above has been "fixed" but maybe not in a good way. It just seems very awkward to keep track of the different objects changing this Queue or that, and what updates what, and what listens to what, etc. I'm going to have read more about what final means and its usage. For now, I left out that keyword, because sometimes I do want to, for instance, reset the commandQueue just by giving it a new reference. – Thufir Sep 4 '13 at 21:17