current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I would be happy to hear feedback on my implementation of HttpClient. I am not so strong with error handling so I want to improve. My thought process behind this was that IOExceptions can be recoverable and should be tried again. Where as client exception isn't. Hope to hear your thoughts.

private HttpResponse execute(int httpVerb, boolean authHeader) throws NoInternetConnectionError, IOException, ClientProtocolException {

    try {
        Log.i(TAG, "setting header and executing");
        switch (httpVerb) {
        case HTTP_POST:
            if (authHeader){
                httpPost.addHeader(header); 
            }
            return httpClient.execute(httpPost);
        case HTTP_PUT:
            if (authHeader){
                httpPut.addHeader(header);
            }
            return httpClient.execute(httpPut);
        case HTTP_GET:
            if (authHeader){
                httpGet.addHeader(header);
            }
            return httpClient.execute(httpGet);
        default:
            return null;
        }
    } catch (IOException e) {
        InternetChecker internetChecker = new InternetChecker(context);
        if (internetChecker.isInternetConnected() == false) {
            throw new NoInternetConnectionError("internet not connected");
        } else {
            numberOfTries ++;
            if (numberOfTries > 2) {
                throw new IOException(e.getMessage());
            } else {
                Log.i(TAG, "executing for the " + String.valueOf(numberOfTries) + " time");
                return execute(httpVerb, authHeader);   
            }
        }
    } finally {
        numberOfTries = 0;
        Log.i(TAG, "finally block run");
    }
}
share|improve this question

2 Answers 2

up vote 2 down vote accepted

  1. I would create a common executeRequest method:

    private HttpResponse executeRequest(final HttpUriRequest request, 
        final String authHeader, final boolean authHeader) {
    if (authHeader){
        request.addHeader(authHeader); 
    }
    return httpClient.execute(request);
}

    then remove the code duplication from the case branches. 

    switch (httpVerb) {
    case HTTP_POST:
        return executeRequest(httpPost, authHeader, header);
    case HTTP_PUT:
        return executeRequest(httpPut, authHeader, header);
    case HTTP_GET:
        return executeRequest(httpGet, authHeader, header);
    default:
        return null;
}

    Anyway, it doesn't seem too good that you have httpPost, httpGet and httpPut at the same time.

  2. Wouldn't it worth to check the result of isInternetConnected() at the beginning of the method? I'm not too familiar with Android development, maybe it's the best to put into the catch block. Anyway, if it is not a slow/costly service I would call at the beginning of the method too.

  3. Calling String.valueOf(numberOfTries) seem unnecessary. I could be simply

    Log.i(TAG, "executing for the " + numberOfTries + " time");

  4. I'd use a loop instead of recursion. Two links on the topic:

share|improve this answer
    
Thanks palacsint. Just a couple of questions if you don't mind. 1. Why is it bad to have all 3 of them at the same time, after all I call the same method (execute) for each 2. The internet connection can be lost at anytime. It could be there at the start when I check then gone once I do the work 3. Didn't know I could put an in in there –  jiduvah Apr 30 '12 at 15:32
    
I have modified my code and its fairly obvious why I shouldn't be using the case. Thanks a lot! –  jiduvah Apr 30 '12 at 15:52
    
1. Because two of the three always unnecessary. Post those parts (or the full code) as a new question and I'll check it. I think only one reference with HttpUriRequest type would be enough but it's hard to say more without the code. 2. Check the update please. –  palacsint Apr 30 '12 at 20:50
up vote 3 down vote

In the end there was no need to retry manually httpclient has a retry handler.

    HttpRequestRetryHandler retryhandler = new DefaultHttpRequestRetryHandler(6, true);

    httpClient.setHttpRequestRetryHandler(retryhandler);
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.