3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ "I really can’t be bothered fixing things and doing it ‘the right way’ for this single purpose." That's a bad attitude to request a code review with. Or did I misunderstand you there? \$\endgroup\$ – Bobby Jan 2 '14 at 11:17
  • \$\begingroup\$ What I mean here is that for a project that no one else is going to see or use, I usually keep a working part working. Is the term 'Refactoring'? I am a little scared of this process, so I either RESTART or leave it alone. What I am trying to achieve here, is doing the right way the first time around. \$\endgroup\$ – bimmo Jan 2 '14 at 22:28
  • \$\begingroup\$ If you're afraid to change something because it could break something, you shouldn't be coding in my opinion. No no no, let me explain: Part of the biggest problems in IT are people which are stuck in the "meh, why change it, it works" mentality. It's not "preserving state" or "keeping it working", it's pushing technical debt and work down the road. Fix things that need to be fixed, refactor everything that needs to be refactored, change names left and right if that makes your code better, fix bugs and problems you introduced...don't be afraid of change, ever. \$\endgroup\$ – Bobby Jan 3 '14 at 8:18
  • \$\begingroup\$ As an analogy, imagine you build a table in your spare time for your dining room. It's not a pretty table and it wobbles slightly when touched, but it is a table you can eat on...should you build or buy a better one? Why should you, you can eat on it, sure from time to time you spill some milk because somebody kicked it and it wobbled so heavily that the glasses fell off, but it works, right? Okay, the leg came off the other day, but a nail fixed it, it works, right? \$\endgroup\$ – Bobby Jan 3 '14 at 8:22
  • \$\begingroup\$ If you're afraid that changes might break something, you should have a look into test driven development. Also I completely understand what you mean by "why should I invest time into something that only I am using?", but the answer is clearly in that case "for yourself"...if you can spare the time, obviously. Rewriting code, refactoring structures, even only thinking about doing it will make you a better coder at the end of the day. Will the code after refactoring be easier to read? Easier to use? Faster? Better? Higher? Don't you want to know if you can do better? \$\endgroup\$ – Bobby Jan 3 '14 at 8:28
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ My awkward code does a number of bizarre things: it has to deal with each set of locks in order. first, to exit the client search, open is set to false and is signaled. That is not the end of the story: now the code waits for the client search to begin again (before alive==false while-loop is checked). to escape this, open is set to true, a signal is sent, and the whole while(alive){} loop is exited. this is obviously unnatural code that solves a problem that the code itself creates. I have never used a has map, and I was wondering about the usual ways to write to the output. thank you. \$\endgroup\$ – bimmo Jan 4 '14 at 6:19
  • \$\begingroup\$ I have since edited some things. I am still at a loss as to what should be done when closing a socket fails: "Sorry sir, I can't delete your important data. You must retain it - there is no other way around it." My new edit throws a Runtime Exception. I guess I am relying on the GC to clean up after me here. \$\endgroup\$ – bimmo Jan 5 '14 at 5:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.