Client-server application

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:

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) {

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:

-
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();

-
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);
    }
}

StackExchange Code Review Q#90818, answer score: 6