Generic HTTP using Android Asynctask

Question

I have developed a generic HTTP functionality using Android Asynctask and Apache HTTP client. Please review the code and let me know if it is the right way of doing or there are other ways to achieve it.

Async class

public class HTTPAsyncTask extends AsyncTask<String, Void, String> {

    private CallBack mCb;
    HashMap<String, String> mData = null;
    List<NameValuePair> mParams= new ArrayList<NameValuePair>();
    String mTypeOfRequest;
    String mStrToBeAppended = "";
    boolean isPostDataInJSONFormat = false;
    JSONObject mJSONPostData = null;

    public HTTPAsyncTask(CallBack c, HashMap<String, String> data, JSONObject jsonObj, String request) {
        mCb = c;
        mTypeOfRequest = request;
        mJSONPostData = jsonObj;
        if((data != null) && (jsonObj == null)){
            mData = data;
            if(mTypeOfRequest.equalsIgnoreCase("GET")){
                Iterator<String> it = mData.keySet().iterator();
                while(it.hasNext()){
                    String key = it.next();
                    mParams.add(new BasicNameValuePair(key, mData.get(key)));
                }
                for (int i = 0; i<mParams.size()-1; i++){
                    mStrToBeAppended+= "?" + mParams.get(i).getName() + "=" + mParams.get(i).getValue() + "&"; 
                }
                //add the last parameter without the "&"
                mStrToBeAppended+= "?" + mParams.get(mParams.size()-1).getName() + "=" + mParams.get(mParams.size()-1).getValue();

            }

            if(mTypeOfRequest.equalsIgnoreCase("POST")){
                    Iterator<String> it = mData.keySet().iterator();
                    while(it.hasNext()){
                        String key = it.next();
                        mParams.add(new BasicNameValuePair(key, mData.get(key)));
                    }
            }

        }
        if ((mData == null) && (jsonObj != null)){
            isPostDataInJSONFormat = true;
        }

    }


    @Override
    protected String doInBackground(String... baseUrls) {

        publishProgress(null);
        if(mTypeOfRequest.equalsIgnoreCase("GET")){
            String finalURL = baseUrls[0]+ mStrToBeAppended;
             return HttpUtility.GET(finalURL);
        }

        if (mTypeOfRequest.equalsIgnoreCase("POST")){
            if(isPostDataInJSONFormat == false){
                return HttpUtility.POST(baseUrls[0],mParams );
            }
            else {
                return HttpUtility.POST(baseUrls[0], mJSONPostData);
            }

        }

        return null;

    }
    // onPostExecute displays the results of the AsyncTask.
    @Override
    protected void onPostExecute(String result) {
       mCb.onResult(result);


   }

    @Override
    protected void onProgressUpdate(Void...voids ) {
        mCb.onProgress();
   }
}

HTTPUtility Class

public class HttpUtility {

    public static String GET(String url){
        InputStream inputStream = null;
        String result = "";
        try {

            // create HttpClient
            HttpClient httpclient = new DefaultHttpClient();

            // make GET request to the given URL
            HttpResponse httpResponse = httpclient.execute(new HttpGet(url));

            // receive response as inputStream
            inputStream = httpResponse.getEntity().getContent();

            // convert inputstream to string
            if(inputStream != null){
                result = convertInputStreamToString(inputStream);
                //inputStream.close();
            }

            else
                result = "Did not work!";

        } catch (Exception e) {
            Log.d("InputStream", e.getLocalizedMessage());
        }

        return result;
    }

    public static String POST(String url, List<NameValuePair> mParams){
        InputStream inputStream = null;
        String result = "";
        try{
            HttpClient httpclient = new DefaultHttpClient();
            HttpPost post = new HttpPost(url);
            post.setEntity(new UrlEncodedFormEntity(mParams, "UTF-8"));
            HttpResponse httpResponse = httpclient.execute(post);

         // receive response as inputStream
            inputStream = httpResponse.getEntity().getContent();

            // convert inputstream to string
            if(inputStream != null){
                result = convertInputStreamToString(inputStream);
                //inputStream.close();
            }

            else
                result = "Did not work!";

        } catch (Exception e) {
            Log.d("InputStream", e.getLocalizedMessage());
        }

        return result;


    }
    public static String POST(String url, JSONObject obj){

        InputStream inputStream = null;
        String result = "";

        HttpClient httpclient = new DefaultHttpClient();

        try{
            HttpPost post = new HttpPost(url);
            post.setHeader("Content-type", "application/json");

            StringEntity se = new StringEntity(obj.toString()); 
            se.setContentEncoding(new BasicHeader(HTTP.CONTENT_TYPE, "application/json"));
            post.setEntity(se);

            HttpResponse httpResponse = httpclient.execute(post);

            // receive response as inputStream
            inputStream = httpResponse.getEntity().getContent();

            // convert inputstream to string
            if(inputStream != null){
                result = convertInputStreamToString(inputStream);
            }

            else
                result = "Did not work!";

        } catch (Exception e) {
            Log.d("InputStream", e.getLocalizedMessage());
        }
        return result;
    }

    public static String convertInputStreamToString(InputStream inputStream) throws IOException{
        BufferedReader bufferedReader = new BufferedReader( new InputStreamReader(inputStream));
        String line = "";
        String result = "";
        while((line = bufferedReader.readLine()) != null)
            result += line;

        inputStream.close();
        return result;

    }
}

Callback interface

public interface CallBack {
    public void onProgress();
    public void onResult(String result);
    public void onCancel();
}

Main activity class

Inside the activity class

final CallBack c = new CallBack(){
    @Override
    public void onProgress() {
        // TODO Auto-generated method stub
        mProgressDialog.show();
    }

    @Override
    public void onResult(String result) {
        // TODO Auto-generated method stub
        mProgressDialog.dismiss();
        mStrResult = result;
        Toast.makeText(getApplicationContext(), mStrResult, Toast.LENGTH_SHORT).show();
    }

    @Override
    public void onCancel() {
        // TODO Auto-generated method stub

    }
};

And then the Asynctask is called inside the Activity like the following:

//For JSON Postdata
String url= "Your URL"

JSONObject postData = new JSONObject();

postData.put(Key1, Data1);
postData.put(Key2, Data2);
HTTPAsyncTask asyncTask = new AsyncTask(mContext,mCallback, null, postData, "POST");
asyncTask.execute(url);

//For Get data

String url = "Your URL";
HashMap getData = new HashMap<Object, Object>();
getData.put("Key",Data);
getData.put("Key",Data));
mGetGCMMessageAsyncTask = new HTTPAsyncTask(mContext, mCallback, getData, null, "GET");
mGetGCMMessageAsyncTask.execute(url);

Answer 1

I'll go over quickly with the minors generals things :

• You should remove TODO Auto-generated method stub when you implemented the method.
• You should always put a modifier for your class variable.
• Don't use the implementation of a class, use the interface Map instead of HashMap
• I don't like the mVariableName notation. (this is subjective)

---

An HTTP Get and Post are very two different things, don't try to mix them up in the same method. It feels to me like you're HTTPAsyncTask should be separated with one abstract class that encapsulate the common code and two implementation with HTTPAsyncTaskGet and HTTPAsyncTaskPost. This would remove one argument to the constructor, better represent the differences between get/post and would help remove duplicate code. You will need to modifiy the implementation of doInBackground but this should simple and will help this method shine with simplicty by removing the if(mTypeOfRequest.equalsIgnoreCase("POST")) conditions. This would be your basic constructor :

public HTTPAsyncTask(CallBack c, HashMap<String, String> data, JSONObject jsonObj) {
    mCb = c;
    mTypeOfRequest = request;
    mJSONPostData = jsonObj;
    mData = data;

    if ((mData == null) && (jsonObj != null)){
        isPostDataInJSONFormat = true;
    }

}

And with your other class you would have :

public HTTPAsyncTaskGet(CallBack c, HashMap<String, String> data, JSONObject jsonObj) {
   super(c,data,jsonObj);
    if((data != null) && (jsonObj == null)){
        Iterator<String> it = mData.keySet().iterator();
        while(it.hasNext()){
            String key = it.next();
            mParams.add(new BasicNameValuePair(key, mData.get(key)));
        }
        for (int i = 0; i<mParams.size()-1; i++){
            mStrToBeAppended+= "?" + mParams.get(i).getName() + "=" + mParams.get(i).getValue() + "&"; 
        }
        //add the last parameter without the "&"
        mStrToBeAppended+= "?" + mParams.get(mParams.size()-1).getName() + "=" + mParams.get(mParams.size()-1).getValue();

        }
    }
}

The high number of arguments for a method is something that must point you to something smelly. It's hard to have high readability when you have a tons of arguments. There is one case where you need data != null and one case jsonObj != null. You should then redefines your constructor to take one or the other, not both. Yes you will probably have more line of codes, but it will be readable. Every constructor will look unique and will have less complexity if you remove those if. Exemple :

public HTTPAsyncTaskGet(CallBack c, JSONObject jsonObj) {
   super(c,null,jsonObj);
}

---

You should not recreate a new DefaultHttpClient(); for every request. You should always try to use the same client. This will help for performance concern and will reduce the complexity of encountering bugs for various reasons (too much connections open, etc).

    try{
        HttpClient httpclient = new DefaultHttpClient();
        HttpPost post = new HttpPost(url);
        post.setEntity(new UrlEncodedFormEntity(mParams, "UTF-8"));
        HttpResponse httpResponse = httpclient.execute(post);

     // receive response as inputStream
        inputStream = httpResponse.getEntity().getContent();

        // convert inputstream to string
        if(inputStream != null){
            result = convertInputStreamToString(inputStream);
            //inputStream.close();
        }

        else
            result = "Did not work!";
   } catch (Exception e) {
       Log.d("InputStream", e.getLocalizedMessage());
   }

This is very prone to errors. You need to always consume the content of the entity, even in case of failure, or your connection won't close and you'll run into problems. Always make sure that you consume your entity. You can use EntityUtils.consume(HttpEntity).

Answer 2

Use a code formatter, one is probably included in your editor. [ctrl+shift+f] in eclipse

Avoid setting variables to null. This turns off warnings and errors and allows you to make mistakes otherwise not possible!

Declare variables as late as possible.

Use final where applicable,

 final String mTypeOfRequest; 

Strings can contain anything. But you have only 2 values (get/post). Consider using a boolean useGet.

Constructor and methods checks ("if") same value. Consider using different classes. One for each "if" outcome. One for get and one for post.

Move the constructor logic to separate method static "constructor" method to ease testing (easy to create object with different contents).

Return as soon as you have done what you want to do. Saving the value for later is confusing at best. All good editors will highlight all exit paths from

Don't return garbage data where you expect good data. Change

  result = "Did not work!";

to

  return ""; or throw a checked exception