Fetch details from server

Question

I am using this code to fetch some details from server from my android app. How can I improve it?

Here I am also transferring some data from one activity to another.

This is how I have written my Fetch_Details class:

public class Fetch_Details extends Activity{

    //JSON Node Names
    private static final String TAG_RESULT = "result";
    private static final String TAG_AID = "id";
    private static final String TAG_PID = "id";

    String sessionId;
    String subject;
    String college;
    String stage;
    String addressedto;
    String address;
    String city;
    String state;
    String aid;
    String pid;
    ArrayList<String> products = new ArrayList<String>();
    ArrayList<String> productids = new ArrayList<String>();
    ArrayList<String> quantities = new ArrayList<String>();
    int count;

    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.fetch_details);

        Intent intent = getIntent();
        sessionId = intent.getStringExtra("sessionId");
        subject = intent.getStringExtra("subject");
        college = intent.getStringExtra("college");
        stage = intent.getStringExtra("stage");
        addressedto = intent.getStringExtra("addressedto");
        address = intent.getStringExtra("address");
        products = intent.getStringArrayListExtra("products");
        quantities = intent.getStringArrayListExtra("quantities");

        System.out.println(products);
        System.out.println(quantities);

       // count = products.size();
        System.out.println("Size of ArrayList:- "+count);

        if (android.os.Build.VERSION.SDK_INT > 9) {
            StrictMode.ThreadPolicy policy = new StrictMode.ThreadPolicy.Builder().permitAll().build();
            StrictMode.setThreadPolicy(policy);
        }


        new AsyncTask<Void, Void, Void>() {

            private ProgressDialog dialog = new ProgressDialog(Fetch_Details.this);
            @Override
            protected void onPreExecute() {
                   dialog.setMessage("Fetching Details... Please wait...");
                   dialog.setCancelable(false);
                   dialog.show();
                  }

            @Override
            protected Void doInBackground(Void... params) {

                // Creating new JSON Parser
                JSONParser jParser = new JSONParser();

                // Getting JSON from ACCOUNT_URL (account=college)
                String urlaccountid = null;
                String accountquery = "Select id from Accounts Where accountname='"+college+"';";

                try{
                    urlaccountid = "https://demo.vtiger.com/webservice.php?operation=query&sessionName="
                             +URLEncoder.encode(sessionId, "UTF-8")
                             +"&query="
                             +URLEncoder.encode(accountquery, "UTF-8");
                } catch (UnsupportedEncodingException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }

                System.out.println("URL_ACCOUNT:- "+urlaccountid);

                JSONObject jsonaccount = jParser.getJSONFromUrl(urlaccountid);              
                try {
                    // Getting JSON Array
                    JSONArray json_result = jsonaccount.getJSONArray(TAG_RESULT);
                    System.out.println(json_result);

                    // Storing  JSON item in a Variable
                    JSONObject resultid = json_result.getJSONObject(0);

                    // Pulling items from the array
                    aid = resultid.getString(TAG_AID);

                } catch (JSONException e) {
                    e.printStackTrace();
                    }

                //Show Response
                  System.out.println(aid);


                // Getting JSON from PRODUCT_URL
                String urlproductid = null;

                for(int i=0;i<=count;i++)
                {
                    String productquery = "select productname,id from Products where productname='"+products.get(i)+"';";

                try{
                    urlproductid = "https://demo.vtiger.com/webservice.php?operation=query&sessionName="
                             +URLEncoder.encode(sessionId, "UTF-8")
                             +"&query="
                             +URLEncoder.encode(productquery, "UTF-8");
                } catch (UnsupportedEncodingException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }

                System.out.println("URL_PRODUCT:- "+urlproductid);

                JSONObject jsonproduct = jParser.getJSONFromUrl(urlproductid);         
                try {
                    // Getting JSON Array
                    JSONArray json_result = jsonproduct.getJSONArray(TAG_RESULT);
                    System.out.println(json_result);

                    // Storing  JSON item in a Variable
                    JSONObject resultid = json_result.getJSONObject(0);

                    // Pulling items from the array
                    pid = resultid.getString(TAG_PID);

                } catch (JSONException e) {
                    e.printStackTrace();
                    }

                //Show Response
                System.out.println(pid);

                productids.add(pid);
                } 


            return null;    
            }                
                    @Override
                    protected void onPostExecute(Void aVoid) {
                        super.onPostExecute(aVoid);    
                        dialog.dismiss();
                       }

                 }.execute();
    }


    public void fifthScreen(View view) 
    {
       Intent intent = new Intent(Fetch_Details.this, Thank_You.class);    
       intent.putExtra("sessionId", sessionId);
       intent.putExtra("subject", subject);
       intent.putExtra("aid", aid);
       intent.putExtra("stage", stage);
       intent.putExtra("addressedto", addressedto);
       intent.putExtra("address", address);
       intent.putStringArrayListExtra("productids", productids);
       intent.putStringArrayListExtra("quantities", quantities);
       System.out.println(productids);
       System.out.println(quantities);
       startActivity(intent);
    }

}

Answer 1

These are some quick thoughts skipping through the code. Nothing android-specific, only regarding Java.

---

Fetch_Details breaks Java conventions. Use camelCase!

---

14 fields in a class is too much and is just begging to be split into several classes. For example, extract a class for adress information.

---

When possible, prefer using interfaces as types over implementations of interfaces.

ArrayList<String> products = new ArrayList<String>();
ArrayList<String> productids = new ArrayList<String>();
ArrayList<String> quantities = new ArrayList<String>();

becomes

List<String> products = new ArrayList<String>();
List<String> productids = new ArrayList<String>();
List<String> quantities = new ArrayList<String>();

productids should probably also be spelled productIds.

---

If you are using a newer Java version, you can save yourself generics in type declarations:

List<String> products = new ArrayList<>();

---

Remove all those // TODO Auto-generated method stub comments. Remove them the second you write any code in that method. If you need a TODO there, write your own.

---

Remove System.out.println calls and commented-out code. Code that is commented out is dead code and printing to stdout is almost never useful. Prefer actual logging instead.

---

Comments like // Creating a new JSON Parser are utterly useless. If someone can't infer that from the code, they have never seen a line of Java in their life.

In general, if you want to write a comment, stop and really think about whether there isn't a better way to make the code comment itself. There pretty much almost always is a way. Comments are inherently bad in the sense that they will never improve, but only get worse and eventually start lying.

---

String accountquery = "Select id from Accounts Where accountname='"+college+"';";

This smells like SQL injections. Also in other places.

---

The formatting in the entire file is off. Formatting is very important. Code that is not formatted properly is automatically bad in my opinion. Use any IDE like Eclipse or IntelliJ and it will do formatting for you. Let the IDE do it's job!

---

e.printStackTrace();

Don't print stacktraces to stdout. Use proper logging, maybe rethrow exceptions. Think about whether ignoring exceptions like this is really a good way to go here.

Answer 2

Here are a few extra points on top of @Ingo's excellent review.

Extract constants

A tip about constructing strings with variables:

urlaccountid = "https://demo.vtiger.com/webservice.php?operation=query&sessionName="
        +URLEncoder.encode(sessionId, "UTF-8")
        +"&query="
        +URLEncoder.encode(accountquery, "UTF-8");

How about this way instead:

// near the top of the file:
private static final WS_SESSION_QUERY_FMT = "https://demo.vtiger.com/webservice.php?operation=query&sessionName=%s&query=%s";

// later in the code:
urlaccountid = String.format(WS_SESSION_QUERY_FMT,
        URLEncoder.encode(sessionId, "UTF-8"),
        URLEncoder.encode(accountquery, "UTF-8"));

In the WS_SESSION_QUERY_FMT variable it's easy to see the full string and it's parameters. When you concatenate parameters piece by piece it's easy to miss a & or other details.

For another thing, it's good to move constants near the top of the file. Even if you use them only once in code, but in your post you're actually using this twice, so it's definitely worth extracting to a constant.

Error handling

Your error handling is practically non-existent:

try{
    urlaccountid = "https://demo.vtiger.com/webservice.php?operation=query&sessionName="
            +URLEncoder.encode(sessionId, "UTF-8")
            +"&query="
            +URLEncoder.encode(accountquery, "UTF-8");
} catch (UnsupportedEncodingException e) {
    // TODO Auto-generated catch block
    e.printStackTrace();
}

After this code, even if there was an exception and the query failed, the method continues to execute as if nothing happened. The lines following after this code will not work, and create many more errors in your log, which will make debugging all the more difficult.

When you catch an exception, you should either respond in some way to ensure that the code after the try-catch block will work correctly, or return from the surrounding block / method. When it's not clear how to handle an exception, consider not catching it, let it bubble up to higher levels, and maybe handle it there, if it makes more sense.

Don't repeat yourself

This piece of code appears twice, with minor differences:

JSONObject jsonaccount = jParser.getJSONFromUrl(urlaccountid);
try {
    // Getting JSON Array
    JSONArray json_result = jsonaccount.getJSONArray(TAG_RESULT);
    System.out.println(json_result);

    // Storing  JSON item in a Variable
    JSONObject resultid = json_result.getJSONObject(0);

    // Pulling items from the array
    aid = resultid.getString(TAG_AID);

} catch (JSONException e) {
    e.printStackTrace();
}

... and soon after:

JSONObject jsonproduct = jParser.getJSONFromUrl(urlproductid);
try {
    // Getting JSON Array
    JSONArray json_result = jsonproduct.getJSONArray(TAG_RESULT);
    System.out.println(json_result);

    // Storing  JSON item in a Variable
    JSONObject resultid = json_result.getJSONObject(0);

    // Pulling items from the array
    pid = resultid.getString(TAG_PID);

} catch (JSONException e) {
    e.printStackTrace();
}

The only difference is in these lines:

aid = resultid.getString(TAG_AID);
// ...
pid = resultid.getString(TAG_PID);

The duplicated piece of code should have been in a separate private method, taking a String parameter (for TAG_AID, TAG_PID) and returning the result (for storing in aid, pid, in the caller).

Copy-pasting code may seem like an easy and fast solution, but it really doesn't ay off in the medium run, I think not even in the short run, really. The real easy way is to actually do it right from the start, and instead of reaching for the Control-C Control-V keys, just extract the similar code to a method with the necessary parameters.