HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaModerate

Java method - levels of abstraction

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
abstractionlevelsjavamethod

Problem

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


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) {
Cli

Solution

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:

-

Socket clientSocket = null;
clientSocket = acceptConnection();


Could be written as

Socket clientSocket = acceptConnection();


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

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

Code Snippets

public void run() {
    try {
        doRun();
    } catch (SomeException e) {
        // exception handling here
    }
}
Socket clientSocket = null;
clientSocket = acceptConnection();
Socket clientSocket = acceptConnection();

Context

StackExchange Code Review Q#40101, answer score: 19

Revisions (0)

No revisions yet.