1
\$\begingroup\$

This question presents two rather small Java programs: the first one is a simple client that reads text from command line and sends it to the server (TextSender.java); the server does nothing more but echoes the received text on the command line (TextReceiver.java)

TextSender.java:

import java.io.IOException;
import java.io.PrintWriter;
import java.net.Socket;
import java.net.UnknownHostException;
import java.util.Scanner;

public class TextSender {

    public static final int PORT = 1857;

    public static void main(String[] args) {
        if (args.length == 0) {
            System.out.println("Usage: java Main IP");
            return;
        }

        try (Socket clientSocket = new Socket(args[0], PORT);
             PrintWriter out = new PrintWriter(clientSocket.getOutputStream(), true);) {
            Scanner scanner = new Scanner(System.in);

            while (scanner.hasNextLine()) {
                String line = scanner.nextLine();

                if (line.trim().equals("quit")) {
                    break;
                }

                // Terminate the line so that the receiver process knows that 
                // the end of input is occured.
                out.print(line + "\n");
                out.flush();
            }
        } catch (UnknownHostException ex) {
            System.err.println("[ERROR] Could not reach host \"" + 
                               args[0] + "\".");
            System.exit(1);
        } catch (IOException ex) {
            System.err.println("[ERROR] IO failed on host \"" + 
                               args[0] + "\".");
            System.exit(1);
        }
    }
}

TextReceiver.java:

import java.io.BufferedReader;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Scanner;
import java.util.logging.Logger;

public class TextReceiver implements Runnable {

    public static final int PORT = 1857;

    public static void main(String[] args) {
        new Thread(new TextReceiver()).start();
    }

    @Override
    public void run() {
        ServerSocket serverSocket = null;
        Socket clientSocket = null;
        BufferedReader in = null;

        try {
            serverSocket = new ServerSocket(PORT);
            System.out.println("[RECEIVER] Waiting for a client.");

            clientSocket = serverSocket.accept();

            in = new BufferedReader(
                    new InputStreamReader(clientSocket.getInputStream()));

            System.out.println("[RECEIVER] A client connected.");

            Scanner scanner = new Scanner(in);

            while (scanner.hasNextLine()) {
                System.out.println(scanner.nextLine());
            }
        } catch (IOException ex) {
            Logger.getLogger("CONNECTION").severe(ex.getMessage());

            if (in != null) {
                close(in, clientSocket, serverSocket);
            } else if (clientSocket != null) {
                close(clientSocket, serverSocket);
            } else if (serverSocket != null) {
                close(serverSocket);
            }
        }

        close(in, clientSocket, serverSocket);
    }

    private void close(Closeable... closeables) {
        for (Closeable c : closeables) {
            try {
                c.close();
            } catch (IOException ex) {

            }
        }
    }
}

Please, tell me anything I could improve.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
  1. Why define PORT as a constant independently in TextReceiver and TextSender? Isn't it the whole point that it's one and the same value? Let's stick to Single Source of Truth then. And if not, why make them public?
  2. I'm not sure if this is necessary:

    if (in != null) {
        close(in, clientSocket, serverSocket);
    } else if (clientSocket != null) {
        close(clientSocket, serverSocket);
    } else if (serverSocket != null) {
        close(serverSocket);
    }
    

    I'd rather have close skip over null values when iterating through closeables.

  3. It could be a better choice to use a finally clause for closing resources (in TextReceiver). It executes regardless of whether there was an exception or not.

  4. If we're logging all other exceptions, I can't see the reason not to log the IOException in close.

\$\endgroup\$
2
  • \$\begingroup\$ 1. I had to create two distinct projects for convenience. I know that repeating oneself ain't good. Otherwise all good points. \$\endgroup\$
    – coderodde
    Nov 3, 2015 at 18:58
  • \$\begingroup\$ @coderodde oh right, it's client-server. I only use Java on Android so I'm not used to that. Well, but then - assuming PORT isn't referred to outside - I'd make it private. Thanks for accepting the answer. \$\endgroup\$ Nov 3, 2015 at 19:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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