Asynchronous network callback code

I did not get the job after submitting this piece of work in an interview, but I have no feedback to know what "BAD" things are inside this block of code.

The requirements are:

• Connect to the server on a known port and IP

• Asynchronously send a message to the server in your choice of format

• Calculate and display the round trip time for each message and the average round trip time for all messages sent

The solution should not be so hard. But I just don't know what's wrong? Bad design? Bad naming? Bad practise?

```
import java.io.BufferedReader;
import java.io.DataOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.Socket;
import java.net.UnknownHostException;

public class EchoClient {

private String hostname;
private int port;
private Socket clientSocket;
private BufferedReader inFromUser, inFromServer;
private DataOutputStream outToServer;
private double averageTime = 0;
private int count = 0;

public EchoClient(String hostname, int port){
this.hostname = hostname;
this.port = port;
try {
this.clientSocket = new Socket(this.hostname, this.port);
} catch (UnknownHostException e) {
System.out.println("Connection Error: unknown host");
System.exit(1);
} catch (IOException e) {
System.out.println("Connection Error: connection refused");
System.exit(1);
}
try{
this.inFromUser = new BufferedReader( new InputStreamReader(System.in));
this.outToServer = new DataOutputStream(this.clientSocket.getOutputStream());
this.inFromServer = new BufferedReader(
new InputStreamReader(this.clientSocket.getInputStream()));
} catch (IOException e) {
System.out.println("Error on Initializing echoclient");
System.exit(1);
}

}

public void start(){
Sy

I think the biggest problem is the lack of synchronization. You modify the averageTime and count variables in the callback which runs concurrently. You should synchronize the access of this variables. There is a good book on this topic: Read the Java Concurrency in Practice, if you have time read it, it's very useful.

Some other things:

-
I don't like inner classes. Reference: Effective Java Second Edition, Item 22: Favor static member classes over.
I would also create an EchoClientMain class which contains the main method and parse the command line parameters. Furthermore, I'd move out to a new file the Callback anonymous inner class and I'd create a Statistics class which would be responsible to calculate and maintain the stats. (Check Single responsibility principle on Wikipedia.)

-
Instead of System.exit() you should rethrow the exceptions. This class is not reusable since a simple error stops the whole application. Just create a custom exception class and throw it:

try {
    this.clientSocket = new Socket(this.hostname, this.port);
} catch (final UnknownHostException uhe) {
    throw new EchoClientException("Connection Error: unknown host", uhe);
}

Let the caller to handle them. In this case the main method should catch the EchoClientException and print its message to the console.

try {
    EchoClient client = new EchoClient(hostname, port);
    client.start();
} catch (final EchoClientException ece) {
    System.err.println(ece.getMessage());
}

-
You should NOT connect to the server in the constructor. I'd do it in the start() method.

-
Close the resources. Create a stop() method which close the opened streams.

-
Check at least for null input parameters: Effective Java, Item 38: Check parameters for validity

Clean Code by Robert C. Martin is also worth reading.

StackExchange Code Review Q#6258, answer score: 22