HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaModerate

HttpURLConnection response code handling

Submitted by: @import:stackexchange-codereview··
0
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:

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 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?



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

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 unstable
for(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.