34
\$\begingroup\$

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(){
        System.out.println("Connecting to " + hostname + " with port No " + port);
        String msgSend;
        try {
            while ((msgSend = inFromUser.readLine()) != null){
                // sendMessage asynchronous
                sendMessage(msgSend, new Callback(){
                    // callback function and calculate the average time
                    public void callback(long timeUsed, String msgReceived){
                        averageTime = (count * averageTime + (timeUsed)) / (count + 1);
                        ++count;
                        System.out.println(msgReceived + 
                            " rtt=" +  (double)Math.round(timeUsed * 100)/100    + " ms" +
                            " artt=" + (double)Math.round(averageTime * 100)/100 + " ms");

                    }
                });    
            }
        } catch (IOException e) {
            System.out.println("Error on reading message from user");
        }
    }

    private void sendMessage(String message, Callback cb){
        Thread sendMessageThread = new Thread(new SendMessageRequest(message, cb));
        sendMessageThread.start();
    }

    interface Callback {
        public void callback(long time, String msg);
    }

    class SendMessageRequest implements Runnable{

        private String message;
        private Callback cb;
        SendMessageRequest(String message, Callback cb){
            this.message = message;
            this.cb = cb;
        }
        @Override
        public void run() {
            String msgReceived;
            long timeStart, timeEnd, timeUsed;
            try {
                timeStart = System.nanoTime();
                outToServer.writeBytes(this.message + '\n');
                msgReceived = inFromServer.readLine();
                timeEnd = System.nanoTime();
                // Calculate the time and get the output
                timeUsed = (timeEnd - timeStart) / 1000000;
                cb.callback(timeUsed, msgReceived);
            } catch (IOException e) {
                System.out.println("Error on sending message to server");
            }

        }

    }

    public static void showUsage(){
        System.out.println("Usage: java EchoClient [hostname] [portNo]");
    }
    /**
     * Entry of the program
     */
            public static void main(String[] args) {
        String hostname = "";
        int port = 0;
        if (args.length < 2){
            showUsage();
            System.exit(0);
        }
        else{
            hostname = args[0];
            port = Integer.parseInt(args[1]);
        }

        EchoClient client = new EchoClient(hostname, port);
        client.start();
    }
}
\$\endgroup\$
  • 9
    \$\begingroup\$ Any company that doesn't provide feedback on a candidate's assignment is a company that wouldn't be worth working for. \$\endgroup\$ – Rontologist Nov 24 '11 at 21:53
  • 10
    \$\begingroup\$ @Rontologist I disagree. First of all, a company would never send a résumé back with feedback, so why should they do it for a code review? It would require the reviewer to spend time annotating reviews for what may be several candidates, and ultimately his decision may come down to preferring someone's code style over another's. Making such a statement on paper would be confrontational at best. \$\endgroup\$ – kojiro Nov 25 '11 at 2:23
  • 2
    \$\begingroup\$ @kojiro A resume is not solicited work and does not belong in the same category. I am of the opinion that if I ask a candidate to do any work (and to create a good source package that is polished with unit tests, etc) than I as an interviewer should be prepared to contact that person and let him know the results, even it if is just high level notes. If there are so many samples that doing that takes too long than you didn't shortlist good enough. Also a code review need not be confrontational (Most people I've phoned with rejections were happy to be given feedback) but can be constructive. \$\endgroup\$ – Rontologist Nov 25 '11 at 18:19
21
\$\begingroup\$

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:

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

  2. 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());
    }
    
  3. You should NOT connect to the server in the constructor. I'd do it in the start() method.

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

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

\$\endgroup\$
6
\$\begingroup\$

I wonder if when they asked for you to send the message asynchronously, they did not want you to launch a second thread. Your second thread does its processing synchronously. Yes its off on another thread, but that may or may not be what they meant by asynchronous. Something like the library here does: https://github.com/sonatype/async-http-client.

As palacsint points out, you haven't synchronized the variables. That my have made it look like you didn't know what you were doing.

The other thing I don't like is that way you handle errors. I'd shut down the application if an error occours, not write it to standard error and try to keep going. I'd also want to display more information about the error, not the simple fact that it happened.

\$\endgroup\$

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.