8
\$\begingroup\$

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.");
\$\endgroup\$
  • \$\begingroup\$ You got labels? Goto style? \$\endgroup\$ – fnc12 Nov 19 '16 at 16:35
11
\$\begingroup\$

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

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

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

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.