Custom networked remote manager

I've spent my fair share of time criticizing other people's code on this site; but I'm finding out I'm kind of a hypocrite. Almost everything I've written so far has been some program whipped together to do some task for me. I have little experience actually starting out writing maintainable code.

Then today I decided to start a semi-large project. I want to write a sort of custom networked remote manager; a kind of cross-breed between ssh and an all-out remote desktop.

I wrote up the following class while realizing the previous revelations:

```
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.Socket;
import java.net.UnknownHostException;
import java.util.ArrayList;

public abstract class Connection implements Runnable {
private final Socket s;
private final InputStream is;
private final OutputStream os;
private final ArrayList posts;
private boolean run;
public Connection(String addr, int port) throws UnknownHostException, IOException {
s = new Socket(addr, port);
is = s.getInputStream();
os = s.getOutputStream();
posts = new ArrayList<>();
run = true;
new Thread(this).start();
}
public void post(byte data[]) {
synchronized (posts) {
posts.add(data);
}
}
public void stop() {
run = false;
}
public void run() {
while (run) {
byte send[] = null;
synchronized (posts) {
if (posts.size() > 0) {
send = posts.get(0);
posts.remove(0);
}
}
if (send != null) {
try {
os.write(send);
} catch (IOException e) {
hasError(e);
}
}
try {
if (is.available() > 0) {
byte data[] = new byte[is.available()];
recei

new Thread(this).start();

I may be wrong about this, but I don't think this is recommended in the constructor.

ExecutorService implementations exist to provider better/consistent handling of asynchronous tasks. Arguably, a better form will be to extend your abstract class and use it with an ExecutorService instance, instead of having it (Connection) decide when to start running.

Inside your run() method, you may want to consider the following rewrite as it is slightly more expressive and reduces one level of nesting:

while (run) {
    byte send[] = null;
    synchronized (posts) {
        if (posts.isEmpty()) {
            continue;
        }
        send = posts.get(0);
        posts.remove(0);
    }
    // send != null probably not required, as long as post() does not accept null
    // if (send != null) {
    try {
        os.write(send);
    } catch (IOException e) {
        hasError(e);
    }
    // }
    // ...
}

Considering you are doing something like:

posts.get(0);
posts.remove(0);

You might actually be looking for a Queue implementation to simplify this approach. In fact, you may be looking for a thread-safe implementation so that you do not need the explicit synchronized blocks, such as something that implements a BlockingQueue.


Given the above sample of my idea of how to make maintainable modular code, what sort of pitfalls should I look out for while making the excursion into a legitimate program of larger than two or three files?

A thorough answer for this will be out-of-scope here on CR, so my only advice is to perhaps ask that on Programmers.SE (as long as you can make it on-topic there). :)

StackExchange Code Review Q#107462, answer score: 11