HttpURLConnection response code handling

Question

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;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.");

• fnc12: You got labels? Goto style?
• Konrad Höffner: @fnc12: I try to refrain using them but sometimes I don't find a better option. That's one of the reasons I put this in code review.

Answer 1

Readability

Let's reformat your code quickly:

 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.");

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'TM.

Functionality

Right, how does this code work? There are a few problems I can (now) see in here:

1. On HTTP_OK, it breaks out of the switch, then it exits the loop (because && !connected), and then immediately disconnects and logs a severe error? This can't be right?
2. HTTP_GATEWAY_TIMEOUT logs a warning.... great, but HTTP_UNAVAILABLE does a System.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.");
 
}

• Konrad Höffner: You are right, the last severe log should be conditional on !connected. I didn't even think about creating a subfunction out of it but it is an excellent idea because I need the same functionality elsewhere. The indentation however I changed to fit it into the box, I use tabs normally.
• user1950349: @rolfl I also have question on http client which I posted here. Just wanted to see if you can help CR my question. I haven't got any response so thought to check with you since I saw you answered lot of http related questions.

Answer 2

Two things to add:

1. I'd print that response code here to the log:

   log.severe(entries + " **unknown response code**.");
   

   It would help debugging.

2. Retry seems a good idea first but one of the recent Java Posse episode (#442 Roundup ‘13 - Big Data Bloopers) has an interesting thought: This might not be that you really want. If there's a problem on the other side it might just makes the thing worse and it probably performs a DoS attack.

• Konrad Höffner: That's why I double the delay after every retry. I hope that is enough to not create a DoS.

Answer 3

Working Solution

 public int callAPI() {
 return 1;
 }
 public int retrylogic() throws InterruptedException, IOException{
 int retry = 0;
 int status = -1;
 boolean delay = false;
 do {
 if (delay) {
 Thread.sleep(2000);
 }
 try {
 status = callAPI();
 }
 catch (Exception e) {
 System.out.println("Error occured");
 status = -1;
 }
 finally {
 switch (status) {
 case 200:
 System.out.println(" **OK**");
 return status; 
 default:
 System.out.println(" **unknown response code**.");
 break;
 }
 retry++;
 System.out.println("Failed retry " + retry + "/" + 3);
 delay = true;
 } 
 }while (retry < 3);
 return status;
 }

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. Please read Why are alternative solutions not welcome?