4
\$\begingroup\$

This is my very simple client-server application. Client sends some commands to the server and server gives back the output to the client. However, my special concern is about the GET command sent to the server. The client request GET filename to download a named file. That file ultimately gets downloaded into the client directory with the HTTP response headers, as I have designed my protocol.

Now I am afraid if my coding follows the protocol accurately, especially the HTTP response headers with the Line break (in both client and server side).

Protocol design:

Protocol

ServerSide:

    package serverside;

import java.io.*;
import java.net.*;
import java.util.Arrays;

public class ServerSide {

    private BufferedReader inputFromClient;
    private PrintWriter outputToClient;
    private FileInputStream fis;
    private OutputStream os;
    private static final int PORT = 8000;
    private ServerSocket serverSocket;
    private Socket socket;

    public static void main(String[] args) {
        int port = PORT;
        if (args.length == 1) {
            port = Integer.parseInt(args[0]);
        }
        new ServerSide(port);
    }

    private boolean fileExists(File[] files, String filename) {
        boolean exists = false;
        for (File file : files) {
            if (filename.equals(file.getName())) {
                exists = true;
            }
        }
        return exists;
    }

    public ServerSide(int port) {
        // create a server socket
        try {
            serverSocket = new ServerSocket(port);
        } catch (IOException ex) {
            System.out.println("Error in server socket creation.");
            System.exit(0);
        }

        while (true) {
            try {

                socket = serverSocket.accept();

                outputToClient = new PrintWriter(socket.getOutputStream());
                inputFromClient = new BufferedReader(new InputStreamReader(socket.getInputStream()));

                while (true) {

                    String request = inputFromClient.readLine();

                    if (!request.startsWith("exit") && !request.startsWith("pwd") && !request.startsWith("list") && !request.startsWith("GET")) {
                        outputToClient.println("Wrong request\r\n"
                                + "\r\n");
                    } else if (request.startsWith("exit")) {
                        break;
                    } else if (request.startsWith("pwd")) {
                        File file = new File(System.getProperty("user.dir"));
                        outputToClient.print("Status OK\r\n"
                                + "Lines 1\r\n"
                                + "\r\n"
                                + "Working dir: " + file.getName() + "\r\n");
                    } else if (request.startsWith("list")) {
                        File file = new File(System.getProperty("user.dir"));
                        File[] files = file.listFiles();
                        outputToClient.print("Status OK\r\n"
                                + "Files " + files.length + "\r\n"
                                + "\r\n"
                                + Arrays.toString(files).substring(1, Arrays.toString(files).length() - 1) + "\r\n");
                    } else if (request.startsWith("GET")) {
                        String filename = request.substring(4);
                        File file = new File(System.getProperty("user.dir"));
                        File[] files = file.listFiles();

                        if (fileExists(files, filename)) {
                            file = new File(filename);
                            int fileSize = (int) file.length();
                            outputToClient.print("Status OK\r\n"
                                    + "Size " + fileSize + " KB" + "\r\n"
                                    + "\r\n"
                                    + "File " + filename + " Download was successfully\r\n");
                            outputToClient.flush();
                            // reading files
                            fis = new FileInputStream(file);
                            os = socket.getOutputStream();
                            byte[] buffer = new byte[2^7-1];
                            int bytesRead = 0;
                            while ((bytesRead = fis.read(buffer))!= -1) {
                                os.write(buffer, 0, bytesRead);
                            }
                            os.close();
                            fis.close();
                        } else {
                            outputToClient.print("Status 400\r\n"
                                    + "File " + filename + " not found\r\n"
                                    + "\r\n");
                            outputToClient.flush();
                        }
                    }
                    outputToClient.flush();
                }
            } catch (IOException e) {
                System.err.println(e);
            }
        }
    }
}

ClientSide:

    package clientside;

import java.io.*;
import java.net.*;
import java.util.Scanner;

public class ClientSide {

    private static Socket socket;
    private static PrintWriter outputToServer;
    private static BufferedReader inputFromServer;
    private static InputStream is;
    private static FileOutputStream fos;
    private static final int PORT = 8000;
    private static final String SERVER = "85.197.159.45";
    boolean Connected;

    public static void main(String[] args) throws InterruptedException {
        String server = "localhost";
        int port = PORT;

        if (args.length >= 1) {
            server = args[0];
        }
        if (args.length >= 2) {
            port = Integer.parseInt(args[1]);
        }

        new ClientSide(server, port);
    }

    public ClientSide(String server, int port) {
        try {
            socket = new Socket(server, port);
            outputToServer = new PrintWriter(socket.getOutputStream(), true);
            inputFromServer = new BufferedReader(new InputStreamReader(socket.getInputStream()));
            System.out.println("Client is connected! ");
            Connected = true;
            String line = null;

            Scanner sc = new Scanner(System.in);
            System.out.print("Type command: ");

            while (sc.hasNextLine()) {
                String request = sc.nextLine();

                if (request.startsWith("exit")) {
                    outputToServer.println(request);
                    System.out.println("Application exited!");
                    //outputToServer.flush();
                    break;
                } else if (request.startsWith("pwd")) {
                    outputToServer.println(request);
                    outputToServer.flush();
                } else if (request.startsWith("list")) {
                    outputToServer.println(request);
                    outputToServer.flush();
                } else if (request.startsWith("GET")) {
                    outputToServer.println(request);
                    outputToServer.flush();
                }
                while (Connected) {
                    line = inputFromServer.readLine();
                    System.out.println(line);
                    if (line.isEmpty()) {
                        Connected = false;
                        if (inputFromServer.ready()) {
                            System.out.println(inputFromServer.readLine());
                        }
                    }
                    if (line.startsWith("Status 400")) {
                        while (!(line = inputFromServer.readLine()).isEmpty()) {
                            System.out.println(line);
                        }
                        break;
                    }
                    if (request.startsWith("GET")) {
                        File file = new File(request.substring(4));
                        is = socket.getInputStream();
                        fos = new FileOutputStream(file);

                        byte[] buffer = new byte[socket.getReceiveBufferSize()];
                        int bytesReceived = 0;
                        while ((bytesReceived = is.read(buffer)) >=0) {
                            //while ((bytesReceived = is.read(buffer))>=buffer) {
                            fos.write(buffer, 0, bytesReceived);
                        }
                        request = "";
                        fos.close();
                        is.close();
                    }
                }
                System.out.print("\nType command: ");
                Connected = true;
            }
            outputToServer.close();
            inputFromServer.close();
            socket.close();
        } catch (IOException e) {
            System.err.println(e);
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Hi, and welcome to Code Review. Thanks for posting your full code, it allows for a more comprehensive review. Nice question. I presume you are reinventing the wheel on purpose? \$\endgroup\$ – rolfl May 15 '15 at 12:43
  • \$\begingroup\$ rolfl, thank you very much. I am pretty new here and I hope I will get some help. Actually I dont get the "reinventing the wheel" issue! Why it's said? By the way my question 1: Is the protocol implementation correct for GET command? Additional question 2: The way I am reading/downloading the file is correct? \$\endgroup\$ – Munira May 15 '15 at 12:46
  • \$\begingroup\$ Reinventing the wheel is "doing something that's been done before". There are a number of libraries available for Java (even one included in Java itself) that handle the HTTP protocol for you. Is there a reason why you are not using those libraries? \$\endgroup\$ – rolfl May 15 '15 at 12:50
  • \$\begingroup\$ rolfl, yes, the reason is I am in intermediate level in java, and for practice purpose I need to implement by my own. I will be very happy if I get any satisfacted answer for this question. Do YOU need my complete protocol design? Because here I just posted GET/ downloading the file part. \$\endgroup\$ – Munira May 15 '15 at 12:52
6
\$\begingroup\$

Ohhh... a juicy question. Your specific concerns are about the GET calls, but let me address some other things first....

General

You have put your entire server-socket loop inside the constructor of the server. Server socket loops are never pretty, and I can understand your uncertainty of how to handle things, but I would suggest one of two things:

  1. set up a separate thread that loops for each accept(), and the Constructor takes the port number, etc. The code would look something like:

    Thread serverThread = new Thread(new ServerSide(port));
    serverThread.start();
    
  2. set up a method on the ServerSide that does the loop, and the code would look like:

    ServerSide server = new ServerSide(port);
    server.listen();
    

I encourage you to explore the Thread version because it will expose you to other aspects of server programming, specifically multi-threading which would be the logical next step for you if you want to handle more than one client at a time.

Bugs

You have some small bugs that concern me... for example:

int fileSize = (int) file.length();
outputToClient.print("Status OK\r\n"
        + "Size " + fileSize + " KB" + "\r\n"
        + "\r\n"
        + "File " + filename + " Download was successfully\r\n");

That code gets the file-size in bytes, but then reports it in KB.

Also, for that code, I would recommend a printf....

outputToClient.printf("Status OK\r\nSize %d KB\r\n\r\nFile %s Download was successfully\r\n",
    fileSize, filename);

Here's an interesting one....:

 byte[] buffer = new byte[2^7-1];

That code creates a buffer of size 4.... which is not what you want.

The ^ is the XOR operator, not the power operator, so 2 XOR 7 is 5, and less 1 is 4. You probably want to use \$2^7\$ which is easiest to do with a shift....

 byte[] buffer = new byte[(1 << 7) - 1];

But, even then, a size 127-size buffer is not huge...

Exceptions

When printing exceptions, always print the trace too. You have code like;

    } catch (IOException ex) {
        System.out.println("Error in server socket creation.");
        System.exit(0);
    }

and

} catch (IOException e) {
    System.err.println(e);
}

Both of those should at least print the entire stacK:

e.printStackTrace();

Also, the System.exit(0) implies the exit is successful, use a non-zero output for an error condition (System.exit(1));

Declare-where-used

You have a number of 'fields' which are declared at the class level, but only used briefly inside specific methods. For example, fis is opened, and closed. There is no need for it to be a class field. The following would be better:

FileInputStream fis = new FileInputStream(....);

try-with-resources

You should become friends with this, it is nice. Your code closes and flushes the streams, which is good, but it would be better to let the system do it automatically.... Your code:

fis = new FileInputStream(file);
os = socket.getOutputStream();
byte[] buffer = new byte[2^7-1];
int bytesRead = 0;
while ((bytesRead = fis.read(buffer))!= -1) {
    os.write(buffer, 0, bytesRead);
}
os.close();
fis.close();

would be better as:

try (FileInputStream fis = new FileInputStream(file);
     OutputStream os = socket.getOutputStream();) {
    byte[] buffer = new byte[2^7-1];
    int bytesRead = 0;
    while ((bytesRead = fis.read(buffer))!= -1) {
        os.write(buffer, 0, bytesRead);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Just overwhelming. I will study your answer with good concentration tonight. A small thanks wouldnt be enough for you. But I wish you all the best moments in your life. Sure I will come back to you if I have something more to ask regarding this post. \$\endgroup\$ – Munira May 15 '15 at 14:17
1
\$\begingroup\$

The test

                if (!request.startsWith("exit") && !request.startsWith("pwd") && !request.startsWith("list") && !request.startsWith("GET"))

is not necessary, and strictly speaking incurs an extra maintenance burden: each time you decide to add another request you'd have to modify this test. Consider a straightforward

                if (request.startsWith("exit")) {
                    handle_exit();
                } else if (request.startsWith("pwd")) {
                    handle_pwd();
                } else if (request.startsWith("list")) {
                    handle_list();
                } else if (request.startsWith("GET")) {
                    handle_get();
                } else {
                    handle_bad_request();
                }

BTW, a cascade of else ifs suggests a map of request to handlers.

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