current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 19 down vote favorite
1

In his Clean Code book, Robert C. Martin states that functions should do only one thing and one should not mix different levels of abstraction. So I started wondering whether the code below meets these requirements (I assume the answer is negative). Which one of the following code snippets is better?

Snippet 1

public void run() {
    isRunning = true;

    try {
        serverSocket = new ServerSocket(port);
    }
    catch (IOException | IllegalArgumentException e) {
        System.out.println("Could not bind socket to given port number.");
        System.out.println(e);
        System.exit(1);
    }

    while(isRunning) {
        Socket clientSocket = null;

        try {
            clientSocket = serverSocket.accept();
        }
        catch (IOException e) {
            logEvent("Could not accept incoming connection.");
        }

        Client client = new Client(clientSocket, this);
        connectedClients.put(client.getID(), client);
        Thread clientThread = new Thread(client);
        clientThread.start();
        logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
        client.send("@SERVER:HELLO");
    }
}

or

Snippet 2

public void run() {
    isRunning = true;

    createServerSocket();

    while(isRunning) {
        Socket clientSocket = null;
        clientSocket = acceptConnection();
        addClient(clientSocket);
    }
}

private void createServerSocket() {
    try {
        serverSocket = new ServerSocket(port);
    }
    catch (IOException | IllegalArgumentException e) {
        System.out.println("Could not bind socket to given port number.");
        System.out.println(e);
        System.exit(1);
    }
}

private Socket acceptConnection() {
    try {
        Socket clientSocket = serverSocket.accept();
    }
    catch (IOException e) {
        logEvent("Could not accept incoming connection.");
    }
    return clientSocket;
}

private void addClient(Socket clientSocket) {
    Client client = new Client(clientSocket, this);
    connectedClients.put(client.getID(), client);
    Thread clientThread = new Thread(client);
    clientThread.start();
    logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
    client.send("@SERVER:HELLO");
}

However, splitting such simple code into multiple fragments seems like an overkill (although it's more readable - is it?). Besides, createServerSockets has side effects (Sys.exit) and addClient does too many things at once. Any advice?

Update. After reading your answers I managed to rewrite the code, so it looks like this:

@Override
public void run() {
    try (ServerSocket serverSocket = createServerSocket()) {
        listenForClients(serverSocket);
    }
    catch (IOException e) {
        System.out.println("Could not bind socket to given port number.");
        System.out.println(e);
        System.exit(1);
    }
}

private ServerSocket createServerSocket() throws IOException {
    return new ServerSocket(port);
}

private void listenForClients(ServerSocket serverSocket) {
    isRunning = true; //Member variable that will be used by a GUI close button.
    while(isRunning) {
        try {
            Socket clientSocket = createClientSocket(serverSocket);
            Client client = createClient(clientSocket);
            startClientThread(client);
            logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
        }
        catch (IOException e) {
            logEvent("Could not accept incoming connection.");
        }
    }
}

private Socket createClientSocket(ServerSocket serverSocket) throws IOException {
    Socket clientSocket = serverSocket.accept();
    return clientSocket;
}

private Client createClient(Socket clientSocket) {
    Client client = new Client(clientSocket, this);
    connectedClients.put(client.getID(), client);
    return client;
}

private void startClientThread(Client client) {
    Thread clientThread = new Thread(client);
    clientThread.start();
}

I think it's much better than the first one, although some changes might be needed. What I like is the readability of run(), better exception-handling and moving the method responsible for sending "hello" message to client-thread code (not showed here).

share|improve this question
add comment

3 Answers

up vote 15 down vote accepted

The second one is much easier to read, it expresses the developers intent. At first sight I (as a maintainer or another developer in the same team, for example) just want a quick overview about the code and don't care about the details. (How the code creates a server socket, for example.) The second one exactly gives that.

The System.exit side effect could be eliminated: throw an exception with a proper message (and don't forget to set the cause). Then handle the exception in the run method. I often see wrappings like the following:

public void run() {
    try {
        doRun();
    } catch (SomeException e) {
        // exception handling here
    }
}

This often makes the doRun() simpler and easier to read.

Some other notes:

  1. Socket clientSocket = null;
clientSocket = acceptConnection();

    Could be written as 

    Socket clientSocket = acceptConnection();

  2. I'd modify the createServerSocket() method to return the server socket and the acceptConnection() method to have a ServerSocket parameter. It would remove the temporal coupling between the two methods. It would be impossible to call them in the wrong order.

  3. I've not written any threading code recently but I don't think that creating a separate thread for every client scale well. You might want to use an executor/thread pool there.

share|improve this answer
1  
Thanks - I like the code you've proposed. Now it seems quite obvious, but I didn't come up with this idea earlier. I suppose I should also split addClient into more methods. –  prometh07 2 days ago
 
@prometh07: Yes, good point. I'd start with a registerNewClient() and a sendInitialMessage() method. –  palacsint 2 days ago
1  
Since clientSocket can't be used anywhere else (i.e., the scope of the variable is so short), I wouldn't even bother with it. Why not just addClient(acceptConnection()) ? –  Joshua Taylor 2 days ago
 
1) Yes. 2) Hmm... the serverSocket is a member variable. It is not used outside the run() method, so you (and rolfl) are right here. 3) The code is for educational purpose only, so I doubt there would be more than 5 users at once (it's a chat application - I will describe it more precisely in main post later). –  prometh07 2 days ago
1  
@JoshuaTaylor: I think that's also fine. Two possible disadvantages: readers don't know the return type of acceptConnection(); using step into while debugging is a little bit harder. –  palacsint yesterday
add comment
up vote 11 down vote

Your exception handling for new ServerSocket(port) could be better. I don't like the way System.exit(1) is buried in a helper function. The exception should be caught in such a way that it bypasses the clientSocket-accepting loop.

Similarly, if serverSocket.accept() throws an exception, you log it, but blithely proceed to try to create a client thread anyway. Instead, the client-thread-creating code should be inside the try block, so that the exception would cause it to be skipped.

Here is a preliminary rearrangement to fix the exception handling.

public void run() {    
    try {
            serverSocket = new ServerSocket(port);

            isRunning = true;
            while (isRunning) {
                    try {
                            Socket clientSocket = serverSocket.accept();
                            Client client = new Client(clientSocket, this);
                            connectedClients.put(client.getID(), client);
                            Thread clientThread = new Thread(client);
                            clientThread.start();
                            logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
                            // The "hello" should be sent by the clientThread code instead
                            // client.send("@SERVER:HELLO");
                    } catch (IOException e) {
                            logEvent("Could not accept incoming connection.");
                    }
            }
    } catch (IOException | IllegalArgumentException e) {
            System.out.println("Could not bind socket to given port number.");
            System.out.println(e);
            System.exit(1);
    }
}

If you would like to offload some responsibility from this method, you could create "client tasks" to be put into a "client processing pool". Then this run() method could be wonderfully generic.

As noted above, client.send("@SERVER:HELLO") belongs in the clientThread code, not in this loop.

share|improve this answer
add comment
up vote 7 down vote

I don't like either solution (or the currently suggested answers). In any application where you run a ServerSocket, I believe that the socket management should be done on the Main thread. This would avoid the exact situation where you have a System.exit(1). The basic pattern for a server application is:

public static void main(String[] args) {
    // sort out the arguments, if any.
    int port = 12345; // default, args can override

    // open the ServerSocket to ensure you can reserve the port befor you do expensive setup.
    try (ServerSocket serverSocket = createServerSocket(port)) {
        // Initialize the system
        setup();

        listenForClients(serverSocket);

    } catch (IOException ioe) {
        // deal with exception
        // can do System.exit(1) in the main thread to set exit codes...
        // but my personal preference is to throw an exception from the main method.
        // whihc exits with 1, and still does a clean shutdown.
    }  
}

There are some things you are doing wrong with your ClientThread setup. If you want the client threads to die when the main thread stops then you should set them to be Daemon threads:

Thread clientThread = new Thread(client);
clientThread.setDaemon(true);

The Java system will terminate when all non-Daemon threads stop. If your only non-Daemon thread is the Main thread (it is always non-Daemon), then the system will exit cleanly when the main thread ends.

You can use this to your advantage in a number of ways... including having a terminating variable like AtomicBoolean isRunning = new AtomicBoolean(true) which any thread can set to false, and the main thread will periodically check... (using a Selector and a ServerSocketChannel - an advanced concept)

The bottom line is that you are not setting Daemon threads appropriately, and as a consequence, you need heavy-weight System.exit(1). Your ServerSocket management should be on the main thread too (or, it is possible, to make it on the only non-Daemon thread by spawning it from the Main thread, and letting the main thread exit normally).

share|improve this answer
1  
I'm going to add GUI later, so I think I need this separate thread. –  prometh07 2 days ago
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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