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

Java HTTP Apache - Making my own library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javaownhttplibrarymakingapache

Problem

My company has asked me to write a few Java based programs to deal with sending HTTP requests and parsing the response. After playing around with the Apache HTTP Commons library and making plenty of shortsighted programs using it, I've decided to make a little library for us to use and avoid major code duplication. We're a pretty small company working with a really old code base, and they don't seem to have time to do code reviews. So I present to you, my small API (leaving major comments outside to describe classes and break up the code segments).

  • As a small aside, you may see reference to static class ResponseBodyDisplay, which is a simple helper class (that looks like crap) to pull out the response body. It's not posted, because I didn't write it and it looks like poop.



```
/**
* This class facilitates an easy to use controller for creating HTTP
* requests without having a bunch of duplicate code.
*
* Typical use:
* HttpController.processBodyRequest(request, json)
*
* In retrospect, "getJSONField()" may be a little outside the scope
* of an HttpController. Meh!
*/

public class HttpController
{ private static DefaultHttpClient httpClient = new DefaultHttpClient();

public static Response processBodyRequest(Request r, String json)
throws IOException
{ r.applyHeaders();
HttpEntityEnclosingRequestBase request = r.getHttpRequest();
ByteArrayEntity dataEntity = new ByteArrayEntity(json.getBytes());
request.setEntity(dataEntity);
return executeRequest(request);
}

private static Response executeRequest(HttpEntityEnclosingRequestBase request)
throws IOException
{ HttpResponse httpResponse = httpClient.execute(request);
Header[] headers = httpResponse.getAllHeaders();
Response r = new Response(httpResponse);
for (Header h: headers)
{ r.addHeader(h.getName(), h.getValue());
}
r.setJSON(ResponseBodyDisplay._getResponseBody(httpRes

Solution

I think that your exception handling is problematic.

try {
    ...
} catch (IOException e) {
    e.printStackTrace();
    return null;
}


Besides logging, all that accomplishes is degrade an "informative" kind of exception into a null, which creates more problems than it solves:

  • The caller would have to explicitly check for a null result. The caller cannot use a try-catch block to handle the error condition, which is the preferred way to deal with such exceptional situations.



  • If the caller forgets to check for a null result, then you'll probably get a NullPointerException at some point. As a developer, which kind of stack trace would you rather try to diagnose: one with an IOException or one with a NullPointerException?



  • Suppose the caller does check for null. What kind of feedback could it present to the user? "Sorry, your request failed for some reason. We can't tell you why." An IOException, on the other hand, can contain more details about why the request failed. You can even choose to handle specific subclasses of IOException and ignore others.



If you catch an exception but aren't able to deal with it effectively, the best thing you could probably do is let it propagate — i.e., don't bother catching it at all in the first place, and declare it in the method signature if appropriate. Alternatively, if you need to avoid a leaky abstraction, you might want to wrap the exception before re-throwing it.

Code Snippets

try {
    ...
} catch (IOException e) {
    e.printStackTrace();
    return null;
}

Context

StackExchange Code Review Q#38926, answer score: 2

Revisions (0)

No revisions yet.