Custom networked remote manager

Question

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<byte[]> 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()];
                    receive(data, is.read(data));
                }
            } catch (IOException e) {
                hasError(e);
            }
        }
    }
    public abstract void receive(byte data[], int len);
    public abstract void hasError(Exception e);
}

It occurred to me I should probably be asking here how maintainable code is actually written. I'm kind of ashamed I haven't done this sort of thing before, but as a self-taught programmer I presume this sort of obstacle is common.

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?

Answer 1

To me, maintainable code is code that I or someone else will be able to understand and modify in the future, even after I've forgotten the details, or even if I'm not around to explain to someone new trying to learn my source code from scratch.

In practice, this mainly means:

• Documenting code. Well-named variables, functions and classes. Copious comments written to be understandable by someone who forgot or never knew what the code did, explaining basic things, thinking, reasons, etc, any place it might not be immediately obvious from the code itself. Nice consistent style.

Your code sample is short and well-formatted and fairly easy to follow, but there are no comments, and some of the variable names are very short (which can be ok if they're only used immediately for a line or two, but would be clearer with explicit names). Those things aren't so important at this scale, but if you're doing something tricky I might not know it, and the longer and more elaborate the project gets, the more documentation can help someone new or returning. Your imports are all self-explainitory by name, but if I'm including something unusual I may say why, e.g:

// Misc imports:
import java.util.concurrent.TimeUnit; // Needed for SpecialSync() function.
import java.wtf.BizarroLib;   // Needed for JimJamFunc();

Depending on the style you choose, the class definition might be commented more like:

  // Definition for class Connection
  //
  // Encapsulates a custom networked remote manager.
  // Throws UnknownHostException if (explain when it can throw).
  // (Maybe explain how to use and clean up, if not trivial.)
  // (Insert class-level notes about when to use it, and any dependencies
  //  or things you might forget in a year, or not know as a new person
  //  seeing this for the first time and wondering what this is.)
  public abstract class Connection implements Runnable {
    private final Socket sock;
    private final InputStream inSock;
    private final OutputStream outSock;
    private final ArrayList<byte[]> posts;
    private boolean shouldRun;
    public Connection(String addr, int port) throws UnknownHostException, IOException { ...

Some IDE's have a format for functions which will cause the comments to appear when the programmer hovers over its name elsewhere in code. Styles vary, but I usually at least like blank lines between function definitions, e.g.:

// Stop processing posts.
public void stop() {
    run = false;
}

// Main processing loop. 
// On IO exceptions, (mention what calling hasError does).
public void run() {
…
}

// Defined elsewhere:
// (Maybe say where to find or put a definition for these.)
public abstract void receive(byte data[], int len);
public abstract void hasError(Exception e);

• Code written in a way that makes sense and is organized, hopefully with few weird quirks and dependencies, and is written with the idea that its requirements may change over time, so it should be made easy to change things. Break code into a meaningful organization of files with organized file names in organized folders. Encapsulate functionality. Make utility functions. For a larger project, usually I build (or re-use) some sort of framework that helps make coding the meat of the logic easier, usually by abstracting out implementation details into nice well-named functions that say what they do and do what I want to do at the level I want to think about it when I'm doing the project's logic only a level or two down from the requirements level.

  Your code sample is short and since you're using Java, you will have one class per file. This one already nicely encapsulates one task. If a class starts to have a lot of different complex functionality, you may want to consider whether it makes sense to divide the class up so that there is at most one dense/complex function per class. I'm more familiar with C, C++ and C#, where a class can be split between files. For Java, it makes sense to organize classes into folders, depending on the project's organization, so I'd expect this class file to be in a place such as org.DomainName.projectName.util. That will start to matter when you have dozens of small classes like this, so you can find this one quickly when you need it (or when you're new and don't what's in the project), but have it out of sight and mind when you don't.

• Complete notes about how to set up the build and production environments, so that if someone just had the files and had to get them running on a blank computer starting with installing the OS and tools, they could do that without being stumped for days by obscure system settings and installation steps.

  So if this sample were part of a larger project, I'd expect these notes to be at the top level of the project file tree, and/or in the project text documentation. If this is a small module meant to be maintainable, you might put it in a comment block at the top of the file, mentioning the build environment and version you developed it with, etc.

• Some design docs that explain how the project files are organized, what the requirements are, and how the operators and end users are supposed to use it, as well as list of current known issues and future plans.

Again, this would be in top-level documentation.

Answer 2

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). :)

Answer 3

CPU utilization

What I noticed is that your run loop keeps spinning around doing nothing when there is no input or output. That is inefficient and will eat up your CPU. One way of fixing this is to use two threads, one doing a blocking read on the inputstream, and one doing a blocking wait on the output queue. That way, when nothing is happening, you won't be using any CPU.

Variable name

You have a variable run with the same name as a method run(). I would recommend renaming the variable to something else.

Answer 4

Think of maintainable code as a tutorial on how to perform the required functionality, including a working example.

Imagine that your audience is another developer, who is about as skilled and experienced as you, but doesn't know why you are writing this code or what you hope to accomplish, until you tell him.

Answer 5

I think your class is not good for unit testing. You've got hidden dependencies inside your constructor that you'll not be able to mock in an easy way. Consider doing a parametrize constructor refactoring.