# Java method - levels of abstraction

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);
logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
client.send("@SERVER:HELLO");
}
}


### Snippet 2

public void run() {
isRunning = true;

createServerSocket();

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

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;
}

Client client = new Client(clientSocket, this);
connectedClients.put(client.getID(), client);
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?

-

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.

-
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 Jan 26 '14 at 17:13
@prometh07: Yes, good point. I'd start with a registerNewClient() and a sendInitialMessage() method. –  palacsint Jan 26 '14 at 17:20
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 Jan 27 '14 at 0:01
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 Jan 27 '14 at 0:07
@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 Jan 27 '14 at 8:53

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);
logEvent("Accepted new connection from " + clientSocket.getRemoteSocketAddress().toString());
// 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.

-

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);

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