Helping my coding style with a java text server

Question

I consider myself a beginner at writing in Java. I have never had a programming class, nor had my code reviewed. I have come to Code Review for some pointers. (Tips that is, not data references.). I have written a simple server/client set, where the server behaves as a textual message relay, receiving and forwarding text between clients. This is not my first attempt at writing socket programs, and I want to know if I am on the right track when it comes to handling data and structuring my code in general. I must warn you, it’s a reasonably big sample.

Before I dump my code on the page, I would like to write some concerns of my own. In some cases, I don’t know or fully understand a better solution. In others I really couldn’t be bothered fixing it. Concerns and questions:

I really can’t be bothered fixing things and doing it ‘the right way’ for this single purpose. It seems, from reading other Code Review posts, code structure is taken very seriously. I try to get the job done in any consistent and logical manner.

I don’t know if I have murdered OO Programming by enclosing SIX classes. Maybe the Entry classes can be condensed into the standard Client and Server objects. (I won’t reuse this code.) Have I fractured these two programs too much?

Is my user input command parsing (in the crudest sense of the word parsing) adequate? Commands don’t accept parameters directly, and must ask for them after processing.

Is there a better method for storing/handling client connections than my custom ClientCollection class? Is the current client disconnection handling any good? I would think a more event-based version would be nicer, but I simply don’t know how.

It has been said that ignoring exceptions is the mark of a naïve programmer. Are (any of) my // ignore filled catch clauses correct?

I feel like a crotchety man faced with the monkey bars when it comes to using Locks and Conditions. Are there holes in my concurrent coordination?

I run NetBeans IDE (Best thing). Is that red line on the right of the source code editor the threshold of too-deep structures? If so, my code is ‘shallow’ enough. The worst parts, I fear, are the client listener of the server, and client message listener of the client. This is due to the control structures concerned with locks. Maybe I could restructure these sections and separate them into individual methods…

My comments are sort of self-moderated and seem insufficient.

I have left streams to be passed by argument, for if I ever want to plug in some sort of ‘custom console’. That’ll never happen. Not with project.

I have posted some samples and a link to the full files on Dropbox. Happy reading. Happy Deciphering.

Command ‘Parsing’:

private void parseCommand(String command){
        //switch statement
        switch(command){
            case "/help":
                writeOutput("\n commands:"
                        + "\n  /help"
                        + "\n  /connect"
                        + "\n  /disconnect"
                        + "\n  /broadcast"
                        + "\n  /exit");
                break;
            case "/connect":
                try{
                    String host = readLine("\nhostname: (IP)\n");
                    int port = readInteger("port to connect to: ");
                    serverHandler.openServer(host, port);
                } catch(IOException | NumberFormatException e){
                    writeOutput("unable to connect");
                }
                break;
            case "/disconnect":
                writeOutput("\ndisconnecting...");
                serverHandler.closeServer();
                break;
        // … …
}

Some awkward code:

public void closeHandler() throws IOException, InterruptedException{
        if(alive){
            alive = false; // kill

            open = false;           // interrupt client search
            if(serverSocket != null){
                serverSocket.close();
            }

            clientListenerLock.lock();
            try{
                clientArrayNotFull.signal();    // end this loop
                open = true;                    // escape the listenerOpens check (thread is dead and
                clientListenerOpens.signal();   // nothing else will happen after this)
            } finally{                          //  alive == false will end thread
                clientListenerLock.unlock();
            }

            join();
        }
   }

My ClientCollection class:

class ClientCollection{
    Client[] clientArray;
    Lock clientArrayLock;
    PrintWriter outputStream;

    ClientCollection(int clientLimit, PrintStream outputStream){
        clientArray = new Client[clientLimit];
        clientArrayLock = new ReentrantLock();
        this.outputStream = new PrintWriter(outputStream, true);
    }

    public void addClient(Socket newClientSocket) throws IOException{
        clientArrayLock.lock();
        try{
            Client newClient = new Client(newClientSocket, this);
            if(!full()){
                int nextSlot = nextAvailableClientSlot();
                clientArray[nextSlot] = newClient;
                newClient.assignIndex(nextSlot);
                newClient.start();
                writeOutput("client " + nextSlot + " added");
            } else{
                newClient.disconnect(); // reject
            }
        } finally{
            clientArrayLock.unlock();
        }
    }

    public void removeClient(int clientIndex){
        clientArrayLock.lock();
        try{
            Client oldClient = clientArray[clientIndex];
            clientArray[clientIndex] = null; // off register
            oldClient.message(">> closed by server");
            oldClient.disconnect();
            writeOutput("client " + clientIndex + " kicked");
        } finally{
            clientArrayLock.unlock();
        }
    }

    public void reportDisconnect(int clientIndex){ // notice no .disconnect()
        if(clientArray[clientIndex] != null){ // ie not already kicked
            clientArrayLock.lock();
            try{
                clientArray[clientIndex] = null;
                writeOutput("client " + clientIndex + " disconnected");
            } finally{
                clientArrayLock.unlock();
            }
        }
    }
    // … …
}

My projects (including plain source code) can be found at https://www.dropbox.com/s/lu4l402gz2a3yyi/messageBroadcast.zip. I would be grateful if someone could give me some feedback.

Answer 1

You have a lot of things in here to review, so I am just going to cherry-pick the things that stand out to me:

• It is unconventional to use the new-line handling you have... It is normal to assume that the 'cursor' is at the start of a line when you print, instead of assuming it is at the end of the previous line. For example, this is 'normal' writeOutput("Hello there\n"); instead of writeOutput("\nHelloThere");. While what you have is not 'wrong', it can lead to odd behavioud on terminals that use the last line of the screen for the prompt, and it may over-write your last message.
• your exception handling is poor. Take for example the line writeOutput("unable to connect"); when you cannot connect to a host. The user (you) will want to know if that is because they mis-typed www.goog1e.com or the port number 8O (intentionally got errors in those). You should at least help the user fix their mistakes.
• in the 'awkward' code you have serverSocket.close(). java.net.ServerSocket is an actual class that is commonly used in Java Server applications. You should avoid using names that make your regular socket appear to be a ServerSocket (which is an entirely different, though related, thing).
• ClientCollection should just be something from java.util.concurrent.* (a ConcurrentHashMap probably). You are reinventing the wheel there, and your implementation is not as robust as others. There is no point in reviewing that code.

In general, your locks follow the pattern (lock-try-finally-unlock) which is good, but, apart from that there is not enough context to determine whether it is right or not. There are some problems, like:

• clientArrayNotFull.signal(); // end this loop What loop?, and why are you signalling that there is space in the Collection when you have done nothing to create space?
• why do you set open to false, and then back to true? There is nothing to suggest that anything will actually see it being false (you do not show us the declaration of open. In other words, this makes no sense: open = false; // interrupt client search ... setting a boolean to false is not going to interrupt anything.... and, if those other things are waiting to get a signal with a 'false' value, well, they will be disappointed because open will be back to true when they get the signal.

In general, you have not given enough context to evaluate the concurrency handling. What you have shown hints at there being other problems too.