3
\$\begingroup\$

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();
\$\endgroup\$
  • 1
    \$\begingroup\$ Could you please show Path class? \$\endgroup\$ – Stan Kurilin Jan 21 '12 at 12:39
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ the question was not only about restructuring the code, but also keep it working in case you decide to do so. this code that you have provided will not work! the problem is that if you open new connection and close it each time (that's what you do, not the original code) the loop iteration ends - the operating system will soon run out of free ports! That's why i've asked to keep code working! \$\endgroup\$ – ilja Jan 22 '12 at 6:51
  • \$\begingroup\$ @ilja First of all you provided that don't even compiled. Secondly, we don't know what it actually do. Beside I believe that reconstruction by decomposition is key idea to improve this code. This was direction, not end solution. \$\endgroup\$ – Stan Kurilin Jan 22 '12 at 8:51

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.