Writing to an HttpURLConnection in a loop

Question

I have this method which sends binary data to server. The code works fine, but still, it's structure is kinda stupid: output and input connections are opened like thousands times (depends on data size), closed only once.

I cannot move the code that opens connection outside of the loop because I'm getting strange errors, like the data was never sent to server.

Please help me to restructure this code so it could still work and have a correct structure:

OutputStream os = null;
StringBuffer messagebuffer = new StringBuffer();
HttpURLConnection huc = null;
DataInputStream dis = null;
InputStream in = null;
Path path = null;
byte[] buf = new byte[64 * 1024];
int bytesRead = 0;

try {
  path = Paths.get("D:\\testfile.rar");
  in = Files.newInputStream(path);

  int i = 0;
  URL u = new URL(defaultURL);

  while ((bytesRead = in.read(buf)) != -1) {

    huc = (HttpURLConnection) u.openConnection(); // wrong 
    huc.setRequestMethod("POST");
    huc.setRequestProperty("chunk-number", i + "");
    huc.setDoOutput(true); // wrong 
    huc.setDoInput(true); // wrong 
    os = huc.getOutputStream(); // wrong 
    os.write(buf, 0, bytesRead);
    os.flush();
    System.out.println(i++ + " " + bytesRead);

    int ch;
    dis = new DataInputStream(huc.getInputStream()); // wrong 

    long len = huc.getContentLength();
    if (len != -1) {
      for (int k = 0; k < len; k++)
        if ((ch = dis.read()) != -1)
          messagebuffer.append((char) ch);
        else {
          // if the content-length is not available
          while ((ch = dis.read()) != -1)
            messagebuffer.append((char) ch);
          }
    }
    Thread.sleep(16);
  }
  dis.close();
  huc.disconnect();

  int statusCode = huc.getResponseCode();
  String message = huc.getResponseMessage();
  messagebuffer.append("status code=" + statusCode + "\n");
  messagebuffer.append("response message=" + message + "\n");

} catch (Exception ex) {
  // throw errors 
} finally {            
  // closing stuff 
}
return messagebuffer.toString();

Answer 1

In my opinion the key way to improve this code is decomposition. Actually, methods with more than 15 java lines are usually smells for me. So

//There should be other signature depending on what does this method actually do
public static String foo(Path path) throws InterruptedException, IOException {
    final StringBuffer messagebuffer = new StringBuffer();
    final byte[] buf = new byte[64 * 1024];
    InputStream in = null;
    try {
        in = Files.newInputStream(path);
        int bytesRead;
        int i = 0;
        while ((bytesRead = in.read(buf)) != -1) {
            dealWithConnection(bytesRead, messagebuffer, buf, i);
            i++;
            Thread.sleep(16);
        }
    } finally {
        closeQuietly(in);
    }
    return messagebuffer.toString();
}


private static void dealWithConnection(int bytesRead, StringBuffer messagebuffer, byte[] buf, int i) throws IOException {
    final URL u = new URL(defaultURL);
    HttpURLConnection huc = null;
    try {
        huc = (HttpURLConnection) u.openConnection();// wrong
        init(huc, i);
        send(huc, buf, bytesRead);
        fillData(huc.getInputStream(), huc.getContentLength(), messagebuffer);
        fillResultInfo(huc, messagebuffer);
    } finally {
        if (huc != null) huc.disconnect();
    }
}

private static void send(HttpURLConnection huc, byte[] buf, int bytesRead) throws IOException {
    OutputStream os = null;
    try {
        os = huc.getOutputStream(); // wrong
        os.write(buf, 0, bytesRead);
        os.flush();
    } finally {
        closeQuietly(os);
    }
}

private static void fillResultInfo(HttpURLConnection huc, StringBuffer messagebuffer) throws IOException {
    int statusCode = huc.getResponseCode();
    String message = huc.getResponseMessage();
    messagebuffer.append("status code=").append(statusCode).append("\n");   //Don't use concatenation when you can append
    messagebuffer.append("response message=" + message + "\n");
}

private static void fillData(InputStream inputStream, int contentLength, StringBuffer messagebuffer) throws IOException {
    DataInputStream dis = null;
    try {
        dis = new DataInputStream(inputStream); // wrong

        //if (contentLength != -1) {    //we don't need this check actually
        int ch;
        for (int k = 0; k < contentLength; k++)
            if ((ch = dis.read()) != -1)
                messagebuffer.append((char) ch);
            else {
                // if the content-length is not available
                while ((ch = dis.read()) != -1)
                    messagebuffer.append((char) ch);
            }
    } finally {
        closeQuietly(dis);
    }
}


private static void init(HttpURLConnection huc, int i) throws ProtocolException {
    huc.setRequestMethod("POST");
    huc.setRequestProperty("chunk-number", i + "");
    huc.setDoOutput(true);
    huc.setDoInput(true); // wrong
}

If you haven't closeQuietly() in your libs, it can be implemented like

private static void closeQuietly(Closeable closeable) {
    if (closeable == null) return;
    try {
        closeable.close();
    } catch (IOException ignored) {
    }
}

It's not so easy task to figure out what does this code do. So further improvement can be easily done. Personally I would start from variables names.

BTW, comments like if the content-length is not available is good signal that there should be some method/variable for this to make code be self documented.