Just need someone familiar with HTTPClient to check over a piece of code

Question

here are two little helper methods I have made for downloading files. I have had to mix and match different tutorials of the web to get what I have here.

Now is there anything that I have done blatantly wrong here?

    public static InputStream simplePostRequest(URL url, List<NameValuePair> postData) throws ClientProtocolException, IOException {
    DefaultHttpClient httpclient = new DefaultHttpClient();  
    HttpPost postMethod=new HttpPost(url.toExternalForm());      
    postMethod.setEntity(new UrlEncodedFormEntity(postData, HTTP.UTF_8));
    HttpResponse response = httpclient.execute(postMethod);
    HttpEntity entity = response.getEntity();

    return entity.getContent();
}




public static InputStream simpleGetRequest(URL url, List<NameValuePair> queryString) throws ClientProtocolException, IOException {

    Uri.Builder uri = new Uri.Builder();
    uri.path(url.getPath());
    for(NameValuePair nvp: queryString) {
        uri.appendQueryParameter(nvp.getName(), nvp.getValue());
    }

    DefaultHttpClient httpClient = new DefaultHttpClient(); 
    HttpHost host = new HttpHost(url.getHost());
    HttpResponse response = httpClient.execute(host, new HttpGet(uri.build().toString()));
    HttpEntity entity = response.getEntity();

    return entity.getContent();
}

Answer 1

I wouldn't expect a huge response to a such a vague question. Why not write a pair of unit tests to try your code out instead?

Anyway, the one thing that stands out for me based on my experience with HttpClient is that if subject to heavy load (large number of concurrent threads), your code seems unsafe - there seems to be no upper bound to the number of concurrent connections that could get created.

If you think this might be relevant in your case, you could try something like this:

class X {

  private static final HttpClient httpClient;

  static {
    SchemeRegistry defaultRegistery = new DefaultHttpClient().getConnectionManager()
                .getSchemeRegistry();
    ThreadSafeClientConnManager connMgr = new ThreadSafeClientConnManager(defaultRegistery);
    connMgr.setMaxTotalConnections(10);
    connMgr.setDefaultMaxPerRoute(10);
    httpClient = new DefaultHttpClient(connMgr);
    httpClient.getParams().setParameter(CoreConnectionPNames.CONNECTION_TIMEOUT, 30000);
  }

  public static InputStream simpleGetRequest(URL url, List<NameValuePair> queryString) throws ClientProtocolException, IOException {...}

  public static InputStream simpleGetRequest(URL url, List<NameValuePair> queryString) throws ClientProtocolException, IOException {...}

}

... and just use the static httpClient in your methods instead of instantiating a new one every time.

I copied and modified this from some code I've written before, but don't consider this necessarily correct. The unit tests will be your friends here.

EDIT: With regards to your comment about mixing URL and the HttpClient library class NameValuePair (is this your concern?), why not just a Map<String, String> in the method signatures?