debugjavaModerate
HttpURLConnection response code handling
Viewed 0 times
coderesponsehandlinghttpurlconnection
Problem
This snippet from a downloader callable handles HTTP status codes. I need critique on both the style (
for or do-while loop better here?) and functionality. Should I manage the delay differently? Do I need to handle InterruptedException specifically?HttpURLConnection connection = null;
boolean connected=false;
outer:
for(int retry=0;retry0) {log.warning("retry "+retry+"/"+RETRIES);Thread.sleep(RETRY_DELAY_MS);}
connection = (HttpURLConnection)entries.openConnection();
connection.connect();
switch(connection.getResponseCode())
{
case HttpURLConnection.HTTP_OK: log.fine(entries +" **OK**");connected=true;break; // fine, go on
case HttpURLConnection.HTTP_GATEWAY_TIMEOUT: log.warning(entries+" **gateway timeout**");break;// retry
case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable
default: log.severe(entries+" **unknown response code**.");break outer; // abort
}
}
connection.disconnect();
log.severe("Aborting download of dataset.");Solution
Readability
Let's reformat your code quickly:
Fixed:
-
1-liners are hard to read:
-
spacing inside for-loop conditions:
-
indentation - 1-space indentation is 'just wrong'™.
Functionality
Right, how does this code work? There are a few problems I can (now) see in here:
The
Suggestion
I recommend a sub-function, with a do-while loop (the following is a quick hack-up - untested):
Let's reformat your code quickly:
HttpURLConnection connection = null;
boolean connected = false;
outer: for (int retry = 0; retry 0) {
log.warning("retry " + retry + "/" + RETRIES);
Thread.sleep(RETRY_DELAY_MS);
}
connection = (HttpURLConnection) entries.openConnection();
connection.connect();
switch (connection.getResponseCode()) {
case HttpURLConnection.HTTP_OK:
log.fine(entries + " **OK**");
connected = true;
break; // fine, go on
case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
log.warning(entries + " **gateway timeout**");
break;// retry
case HttpURLConnection.HTTP_UNAVAILABLE:
System.out.println(entries + "**unavailable**");
break;// retry, server is unstable
default:
log.severe(entries + " **unknown response code**.");
break outer; // abort
}
}
connection.disconnect();
log.severe("Aborting download of dataset.");Fixed:
-
1-liners are hard to read:
case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable-
spacing inside for-loop conditions:
for(int retry=0;retry<=RETRIES&&!connected;retry++)-
indentation - 1-space indentation is 'just wrong'™.
Functionality
Right, how does this code work? There are a few problems I can (now) see in here:
- On HTTP_OK, it
breaksout of the switch, then it exits the loop (because&& !connected), and then immediately disconnects and logs a severe error? This can't be right?
HTTP_GATEWAY_TIMEOUTlogs a warning.... great, butHTTP_UNAVAILABLEdoes aSystem.out.println
The
InterruptedException handling is not shown here. There are articles on how to do this properly. Google up on that.Suggestion
I recommend a sub-function, with a do-while loop (the following is a quick hack-up - untested):
private static final HttpURLConnection getConnection(URL entries) throws InterruptedException{
int retry = 0;
boolean delay = false;
do {
if (delay) {
Thread.sleep(RETRY_DELAY_MS);
}
HttpURLConnection connection = (HttpURLConnection)entries.openConnection();
switch (connection.getResponseCode()) {
case HttpURLConnection.HTTP_OK:
log.fine(entries + " **OK**");
return connection; // **EXIT POINT** fine, go on
case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
log.warning(entries + " **gateway timeout**");
break;// retry
case HttpURLConnection.HTTP_UNAVAILABLE:
log.warning(entries + "**unavailable**");
break;// retry, server is unstable
default:
log.severe(entries + " **unknown response code**.");
break; // abort
}
// we did not succeed with connection (or we would have returned the connection).
connection.disconnect();
// retry
retry++;
log.warning("Failed retry " + retry + "/" + RETRIES);
delay = true;
} while (retry < RETRIES);
log.severe("Aborting download of dataset.");
}Code Snippets
HttpURLConnection connection = null;
boolean connected = false;
outer: for (int retry = 0; retry <= RETRIES && !connected; retry++) {
if (retry > 0) {
log.warning("retry " + retry + "/" + RETRIES);
Thread.sleep(RETRY_DELAY_MS);
}
connection = (HttpURLConnection) entries.openConnection();
connection.connect();
switch (connection.getResponseCode()) {
case HttpURLConnection.HTTP_OK:
log.fine(entries + " **OK**");
connected = true;
break; // fine, go on
case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
log.warning(entries + " **gateway timeout**");
break;// retry
case HttpURLConnection.HTTP_UNAVAILABLE:
System.out.println(entries + "**unavailable**");
break;// retry, server is unstable
default:
log.severe(entries + " **unknown response code**.");
break outer; // abort
}
}
connection.disconnect();
log.severe("Aborting download of dataset.");case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstablefor(int retry=0;retry<=RETRIES&&!connected;retry++)private static final HttpURLConnection getConnection(URL entries) throws InterruptedException{
int retry = 0;
boolean delay = false;
do {
if (delay) {
Thread.sleep(RETRY_DELAY_MS);
}
HttpURLConnection connection = (HttpURLConnection)entries.openConnection();
switch (connection.getResponseCode()) {
case HttpURLConnection.HTTP_OK:
log.fine(entries + " **OK**");
return connection; // **EXIT POINT** fine, go on
case HttpURLConnection.HTTP_GATEWAY_TIMEOUT:
log.warning(entries + " **gateway timeout**");
break;// retry
case HttpURLConnection.HTTP_UNAVAILABLE:
log.warning(entries + "**unavailable**");
break;// retry, server is unstable
default:
log.severe(entries + " **unknown response code**.");
break; // abort
}
// we did not succeed with connection (or we would have returned the connection).
connection.disconnect();
// retry
retry++;
log.warning("Failed retry " + retry + "/" + RETRIES);
delay = true;
} while (retry < RETRIES);
log.severe("Aborting download of dataset.");
}Context
StackExchange Code Review Q#45819, answer score: 12
Revisions (0)
No revisions yet.