6
\$\begingroup\$

I have developed this simple TCP Server/Client communication program, and am looking for ways of lowering the code footprint where possible.Side note, Both Classes are Main as they run independently, though interact.

TCPClient

package socketUse;
import java.io.BufferedReader;
import java.io.DataOutputStream;
import java.io.InputStreamReader;
import java.net.Socket;
import java.net.SocketTimeoutException;

class TCPClient
{
    public static void main(String[] args) throws Exception
    {
        try
        {
        String input;
        String modedInput;
        System.out.println("You are connected to the TCPCLient;" + "\n" + "Please enter a message:");
        BufferedReader inFromUser = new BufferedReader(new InputStreamReader(System.in));
        @SuppressWarnings("resource")
        Socket client = new Socket("localhost", 22003);
        client.setSoTimeout(3000);
        while(true)
        {
            DataOutputStream outToServer = new DataOutputStream(client.getOutputStream());
            BufferedReader inFromServer = new BufferedReader(new InputStreamReader(client.getInputStream()));
            input = inFromUser.readLine();
            outToServer.writeBytes(input + '\n');
            modedInput = inFromServer.readLine();
            System.out.println("You Sent: " + modedInput);
            try
            {
                Thread.sleep(2000);
            }
            catch(InterruptedException e)
            {
                Thread.currentThread().interrupt();
            }
        }
        }
        catch(SocketTimeoutException e)
        {
            System.out.println("Timed Out Waiting for a Response from the Server");
        }

    }
}

TCPServer

package socketUse;
import java.io.BufferedReader;
import java.io.DataOutputStream;
import java.io.InputStreamReader;
import java.net.ServerSocket;
import java.net.Socket;

class TCPServer
{
    public static void main(String[] args) throws Exception
    {
        try
        {
            String TCPclientMess;
            String returnMess;
            @SuppressWarnings("resource")
            ServerSocket input = new ServerSocket(22003);

            while(true)
            {
                Socket connectionSocket = input.accept();
                BufferedReader iptFromClient = new BufferedReader(new InputStreamReader(connectionSocket.getInputStream()));
                DataOutputStream optToClient = new DataOutputStream(connectionSocket.getOutputStream());
                TCPclientMess = iptFromClient.readLine();
                System.out.println("You Sent: " + TCPclientMess);
                returnMess = TCPclientMess;     
                optToClient.writeBytes(returnMess);
            }
        }

        catch(Exception e)
        {
            e.getStackTrace();
        }
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Efficiency / logic

In the while loop in the client class, you recreate a DataOutputStream and a BufferedReader from the socket in every single iteration. This is just silly. It would be better to create these objects before the loop.

Closing resources

You didn't close any of the resources you open. As of Java 7, an easy way to properly close everything is using the try-with-resources idiom, for example:

try (
    Socket connectionSocket = input.accept();
    BufferedReader iptFromClient = new BufferedReader(new InputStreamReader(connectionSocket.getInputStream()));
    DataOutputStream optToClient = new DataOutputStream(connectionSocket.getOutputStream())
) {
    String TCPclientMess = iptFromClient.readLine();
    System.out.println("You Sent: " + TCPclientMess);
    String returnMess = TCPclientMess;
    optToClient.writeBytes(returnMess);
}

Limit variables to the smallest necessary scope

In both implementation you declare some variables at the top, when that's really not needed and may invite mistakes. It's best to declare variables at the smallest scope where you really need them. For example, instead of:

    String input;
    // ...
    while(true)
    {
        BufferedReader inFromServer = new BufferedReader(new InputStreamReader(client.getInputStream()));
        input = inFromUser.readLine();
        // ...

This is better:

    while(true)
    {
        BufferedReader inFromServer = new BufferedReader(new InputStreamReader(client.getInputStream()));
        String input = inFromUser.readLine();
        // ...

Hardcoded values

Instead of hardcoding "localhost", 22003, 3000, it would be better to put these values in constants with descriptive names, for example:

private static final int TIMEOUT_MILLIS = 3000;
private static final String SERVER_HOST = "localhost";
private static final int SERVER_PORT = 22003;

Redundant variable

In TCPServer, the message read from the client input stream is stored in the TCPclientMess variable, and then assigned to the returnMess variable, which is not used for anything else. You don't need both variables, one of them would be enough.

Naming

You have a String named TCPclientMess. The convention is to use camelCase for variable names.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ That's exactly what I was looking for, Thank You so much! \$\endgroup\$ Commented Jan 28, 2015 at 23:16
  • \$\begingroup\$ After reviewing the new code, I have realised that, in this exercise there is no need to close the resource on TCPServer, as this is supposed to run continually until a shut-down command is sent (which I have now added), but it is something to consider for future projects. \$\endgroup\$ Commented Jan 29, 2015 at 9:25

Your Answer

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

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