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

• Stan Kurilin: Could you please show Path class?

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.

• ilja: 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!
• Stan Kurilin: @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.