Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Functionality

Functionality

##Functionality

Functionality

added 22 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238
  • 1-liners are hard to read: case HttpURLConnection.HTTP_UNAVAILABLE: System.out.println(entries+ "**unavailable**");break;// retry, server is unstable

    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++)

    spacing inside for-loop conditions:

     for(int retry=0;retry<=RETRIES&&!connected;retry++)
    
  • indentation - 1-space indentation is 'just wrong'TM.

    indentation - 1-space indentation is 'just wrong'TM.

  • 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.
  • 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.

Source Link
rolfl
  • 98.1k
  • 17
  • 219
  • 419

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

AltStyle によって変換されたページ (->オリジナル) /