6
\$\begingroup\$

The following working function seems a bit hacky to me as it has been pieced together from a handful of (probably poorly written) online tutorials, and I want to know if there is a better (or more standard) way to approach this.

/***
 * GET HTTP Operation
 * 
 * @param   request Request URI 
 * @return  Element Root Element of XML Response
 */
protected Element get(String request) {

    HttpURLConnection connection = null;

    Element rootElement;

    try {

        URL url = new URL(this.baseUrl + request);
        connection = (HttpURLConnection) url.openConnection();
        connection.setRequestMethod("GET");
        BASE64Encoder enc = new sun.misc.BASE64Encoder();
        String userpassword = this.username + ":" + this.password;
        String encodedAuthorization = enc.encode( userpassword.getBytes() );
        connection.setRequestProperty("Authorization", "Basic "+ encodedAuthorization);
        connection.setRequestProperty("Content-type", "application/xml");
        connection.setRequestProperty("Accept", "application/xml");

        InputStream responseStream = connection.getInputStream();

        //--- Parse XML response InputStream into DOM

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();

        Document doc = db.parse(responseStream);

        rootElement = doc.getDocumentElement();

    } catch(Exception e) {

        System.out.print(e.toString());
        rootElement = null;

    } finally {

        if(connection != null) {
            connection.disconnect(); 
        }

    }

    return rootElement;
}

I am a Java novice and am especially unclear on proper try/catch usage.

I am creating a Java Wrapper for an REST API and this is a small piece of that puzzle. You can click here to see a bigger picture of the project as a whole.

\$\endgroup\$
2
\$\begingroup\$

Adding another answer since I'm actually looking at the code now...

You are swallowing every exception that gets thrown. The calling code has no idea whether it tried to hit a page that didn't exist, its request was malformed, etc. If you don't want to expose the exceptions (probably a good idea so your client code doesn't have to worry), then you might want to set some sort of error flag as part of your class that can be probed for the cause of not getting back the XML, or throw your own custom exceptions.

I'm assuming in your other methods you do similar sorts of set up for the connection (the accepts, etc)...I'd recommend moving that out into its own setup method. If you need to specialize based on the type of request, then pass in the type and you can at least consolidate all of that into one place.

\$\endgroup\$
  • \$\begingroup\$ ok, so form a single 'request' method and pass the verb in as an ENUM type. I completely get what you are saying about the try/catch being too broad, in a request like this are there key lines you would expect to see an exception thrown? \$\endgroup\$ – jondavidjohn Mar 24 '11 at 16:09
  • \$\begingroup\$ would you just handle the need to send request data with a third argument, and just pass it null if it is a request that does not need request data? GET for instance. \$\endgroup\$ – jondavidjohn Mar 24 '11 at 16:10
  • \$\begingroup\$ @jondavidjon: I'd keep the get/put/delete/post, and have those deal with the underlying request method like you said. I'd expect anything that was not under my control to throw (ill-formed URL, connection failing to the remote host, bad xml, etc) \$\endgroup\$ – Mark Loeser Mar 24 '11 at 16:13

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.