# HttpURLConnection response code handling

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

• You got labels? Goto style? – fnc12 Nov 19 '16 at 16:35
• @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. – Konrad Höffner Aug 1 at 15:05

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


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:

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) {
}
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);

}

• 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. – Konrad Höffner Mar 31 '14 at 14:33
• @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. – user1950349 May 20 '16 at 18:42

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.

• That's why I double the delay after every retry. I hope that is enough to not create a DoS. – Konrad Höffner Apr 4 '14 at 9:02

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) {
}

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;
}

• 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? – Sᴀᴍ Onᴇᴌᴀ Aug 1 at 15:22