JSON fetcher for Eclipse plugin

Question

This is a client module of an Eclipse plugin. I am planning to use this code as a "good exception handling" code example in one of my papers. How does it look?

HttpURLConnection httpconn=null;
BufferedReader breader=null;
try{    
    URL url=new URL(this.web_service_url);
    httpconn=(HttpURLConnection)url.openConnection();
    httpconn.setRequestMethod("GET");
    System.out.println(httpconn.getResponseMessage());
    if(httpconn.getResponseCode()==HttpURLConnection.HTTP_OK)
    {
        breader=new BufferedReader(new InputStreamReader(httpconn.getInputStream()));
        String line=null;
        while((line=breader.readLine())!=null)
        {
            jsonResponse+=line;
        }
        //System.out.println(jsonResponse);
        //display_json_results(jsonResponse);
    }
}catch(MalformedURLException e){
    MessageDialog.openError(Display.getDefault().getShells()[0], "Invalid URL", e.getMessage());
}catch(ProtocolException e){
    MessageDialog.openError(Display.getDefault().getShells()[0], "Invalid Protocol", e.getMessage());
}
catch(IOException e2){
    Log.info("Failed to access the data"+e2.getMessage());
}
finally{
    try{
    breader.close();}catch(IOException e){
        Log.info("Failed to release resources"+e.getMessage());
    }

}

Answer 1

General

As example code, there is a fair amount to comment on. For an eclipse plugin, I would at least expect you to select-all and Ctrl-Shift-F ....

1. consistent use of braces (on the end of the line, not start of the new line)
2. consistent spacing between values and operators jsonResponse+=line; to jsonResponse += line;

The formatted code looks like:

    HttpURLConnection httpconn = null;
    BufferedReader breader = null;
    try {
        URL url = new URL(this.web_service_url);
        httpconn = (HttpURLConnection) url.openConnection();
        httpconn.setRequestMethod("GET");
        System.out.println(httpconn.getResponseMessage());
        if (httpconn.getResponseCode() == HttpURLConnection.HTTP_OK) {
            breader = new BufferedReader(new InputStreamReader(
                    httpconn.getInputStream()));
            String line = null;
            while ((line = breader.readLine()) != null) {
                jsonResponse += line;
            }
            // System.out.println(jsonResponse);
            // display_json_results(jsonResponse);
        }
    } catch (MalformedURLException e) {
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid URL", e.getMessage());
    } catch (ProtocolException e) {
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid Protocol", e.getMessage());
    } catch (IOException e2) {
        Log.info("Failed to access the data" + e2.getMessage());
    } finally {
        try {
            breader.close();
        } catch (IOException e) {
            Log.info("Failed to release resources" + e.getMessage());
        }

    }

Working with the formatted code now:

• Why do you have an active System.out.println(httpconn.getResponseMessage()); in the code? That should be commented out. Use the Log for that.
• Why are the HttpURLConnection and BufferedReader declared outside the try-block? There is no need.
• The BufferedReader should be opened with a try-with-resource block to perform the auto-close.
• you are losing newlines on the BufferedReader's readLine(). You should use a different system, or alternatively add the newline back in... unless you are using some other mechanism to reformat thte JSON.
• you should be appending to a StringBuilder, not doing String concatenation (jsonResonse += line; == BAD)
• why have commented-out code in example code? Get rid of the println's and the display_json_results

Exception Handling

This exception-handling has a lot of problems I can see.

1. you are throwing away all stack traces.... you do not report them! Why?

2. you have different forms of exception naming in your handlers. Two of them call the exception e. The third is called e2. Use meaningful names, or use consistent names. The mix is.... mixed up.

3. Calling Log.info("Failed to access the data" + e2.getMessage()); is ... poor. If you have the Log available, it should at least be a warning! Also, you should pass the full exception to the Log, and log the full trace. Finally, you do not have a space between the 'data' and the e2.getMessage() in the output... .... access the data" should be ... access the data: "

4. When there is an exception, you should help the user by indicating what data was causing the exception. In this case, the errors/dialogs should contain this.web_service_url since that was the source of the problem.

5. Since you have the log, not only should you be outputting the exception message for the URL formats to the Display, but also to the Log.

Quick reformat

I messed with the code, and got the following:

  try {
        URL url = new URL(this.web_service_url);
        HttpURLConnection httpconn = (HttpURLConnection) url.openConnection();
        httpconn.setRequestMethod("GET");
        //System.out.println(httpconn.getResponseMessage());
        if (httpconn.getResponseCode() == HttpURLConnection.HTTP_OK) {
            try (BufferedReader breader = new BufferedReader(new InputStreamReader(
                    httpconn.getInputStream()));) {
                String line = null;
                while ((line = breader.readLine()) != null) {
                    jsonResponse.append(line).append("\n");
                }
            }
            // System.out.println(jsonResponse);
            // display_json_results(jsonResponse);
        }
    } catch (MalformedURLException mue) {
        Log.warn("Invalid URL " + this.web_service_url, mue);
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid URL " + this.web_service_url, mue.getMessage());
    } catch (ProtocolException pe) {
        Log.warn("Protocol Exception " + this.web_service_url, pe);
        MessageDialog.openError(Display.getDefault().getShells()[0],
                "Invalid Protocol " + this.web_service_url, pe.getMessage());
    } catch (IOException ioe) {
        Log.warn("Failed to access the data " + this.web_service_url, ioe);
    }

}

Answer 2

The Java issues are already described above, so let me add Eclipse-specific points.

1. This code will handle exceptions as expected in the main thread only (and will throw InvalidThreadAccess otherwise). But it is not a good idea to work with I/O in the main thread as the user will be unhappy with a frozen UI.

2. Showing dialogues from some operation is bad practice. One day you will get a very long chain of error dialogues because something is wrong with the URL or network address.

3. Consider using an org.ecipse.core.runtime.IStatus to describe the errors. Also, it will be good to pass some "reporter" instance to this method to decouple error reporting.