JSON AsyncTask refactoring to improve quality? I'm new to Android. I was wondering if anyone had any suggestions. It's basically an AsyncTask to get a remote JSON array and put it into a native array.

Would be very grateful for suggestions.

package com.example.test2;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.Arrays;
import java.util.Hashtable;

import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;

import android.app.Activity;
import android.os.AsyncTask;
import android.os.Bundle;
import android.widget.TextView;

public class MainActivity extends Activity {

private String                  strCategoryJsonUrl      = "http://www.mydomain.com/categories.json";
getCategories                   myCategories;


@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    myCategories            = new getCategories();
    myCategories.execute();
}//onCreate

private class getCategories extends AsyncTask&lt;Integer, Integer, String&gt;{

    @Override
    protected String doInBackground(Integer... intMyGuessLevel) {
        String strJson  = "";

        try {
            URL url = new URL(strCategoryJsonUrl);
            HttpURLConnection con = (HttpURLConnection) url.openConnection();
            strJson = readStream(con.getInputStream());
        } catch (Exception e) {
            e.printStackTrace();
        }

        return strJson;
    } //doInBackground

    private String readStream(InputStream in) {
        BufferedReader  reader      = null;
        String          strContent  = "";

        try {
            reader = new BufferedReader(new InputStreamReader(in));
            String line = "";
            while ((line = reader.readLine()) != null) {
                strContent  = strContent + line;
            }
        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            if (reader != null) {
                try {
                    reader.close();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        } //finally

        return strContent;
    } //readStream

    protected void onPostExecute(String strResult) {
        int i                   = 0;
        String strValue         = "";
        TextView txtDebug       = (TextView) findViewById(R.id.txtdebug);
        String arrCategories[]  = new String[1];

        //parse JSON data
        try{
            JSONArray   jArray  = new JSONArray(strResult);
            //JSONObject  menu = jObject.getJSONObject("menu");
            arrCategories       =   new String[jArray.length()];

            for(i=0; i &lt; jArray.length(); i++) {
                JSONObject jObject  = new JSONObject(jArray.get(i).toString());

                strValue            = jObject.getString("name");

                //put value into array
                arrCategories[i]    = strValue;
            }

            //debug output the array
            txtDebug.setText(Arrays.toString(arrCategories));


        } catch (JSONException e) {
            e.printStackTrace();
            txtDebug.append("Error in categories json file \n");
            txtDebug.append(String.valueOf(e));
        } // catch 


    } //onPostExecute
} //getJsonContent

}
Don't open a resource in one place and close it in another. Keep the opening and closing of a resource localized to the same function (if you open it, you close it). This makes it easier to spot potential resource leaks.

For example, an InputStream is being opened in doInBackground(), but it is being closed in readStream(). Since doInBackground() opened the resource, it should be responsible for closing it.



HttpURLConnection con = (HttpURLConnection)url.openConnection();


You are not using anything specific to HttpURLConnection, so there is no reason to cast it. This works just fine:

URLConnection con = url.openConnection();




try {
    ...
} catch (Exception e) {
    e.printStackTrace();
}


Don't ignore exceptions. Either let them continue up to the caller, or handle them appropriately. The readStream() function is a good example of where the exception should continue to the caller. Declare it as throwing IOException and remove the exception handling:

private String readStream(InputStream in) throws IOException {
    String content = "";
    BufferedReader reader = new BufferedReader(new InputStreamReader(in));
    String line = null;
    while ((line = reader.readLine()) != null) {
        content += line;
    }
    return content;
}




JSONObject jObject = new JSONObject(jArray.get(i).toString());


This can be simplified to this:

JSONObject jObject = jArray.getJSONObject(i);




Declare variables closer to there they are used. For example, in onPostExecute(): i, strValue and arrCategories are all declared at the top of the function, but are only ever used inside the try block.

protected void onPostExecute(String strResult) {
    TextView txtDebug = (TextView)findViewById(R.id.txtdebug);
    try {
        JSONArray jArray = new JSONArray(strResult);
        String[] arrCategories = new String[jArray.length()];
        for (int i = 0; i &lt; jArray.length(); i++) {
            JSONObject jObject = jArray.getJSONObject(i);
            arrCategories[i] = jObject.getString("name");
        }
        txtDebug.setText(Arrays.toString(arrCategories));
    } catch (JSONException e) {
        e.printStackTrace();
        txtDebug.append("Error in categories json file \n");
        txtDebug.append(String.valueOf(e));
    }
}




I would also like to see onPostExecute() broken down further. It is currently doing two things: parsing the JSON array and updating the UI. Move those into their own functions.



I am not a big fan of the comments in this code. I do not believe they add any value, and are only a distraction while reading.