patternjavaMinor
Writing to an HttpURLConnection in a loop
Viewed 0 times
loopwritinghttpurlconnection
Problem
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 stu
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 stu
Solution
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
If you haven't
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
//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.Code Snippets
//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
}private static void closeQuietly(Closeable closeable) {
if (closeable == null) return;
try {
closeable.close();
} catch (IOException ignored) {
}
}Context
StackExchange Code Review Q#8051, answer score: 2
Revisions (0)
No revisions yet.