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 4 down vote favorite

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<Integer, Integer, String>{

    @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 < 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

}
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

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 < 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.

share|improve this answer
add comment

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.