Multiple background tasks

Question

This is taken from my post at Stack Overflow here.

I need your help to review my code for improvement and best practice.

  private void logSessionToServer() {
    // TODO Auto-generated method stub

    Thread thread = new Thread(){
        public void run(){
            strUsername = etUsername.getText().toString();

             // Building Parameters
            List<NameValuePair> params = new ArrayList<NameValuePair>();
            params.add(new BasicNameValuePair( TAG_USERNAME, strUsername ));

            // getting JSON Object
            // Note it accepts POST method only
            JSONObject json = jsonParser.makeHttpRequest(url_log_session,
                "POST", params);

            // check log cat from response
            Log.d("Create Response", json.toString());

            // check for success tag
            try {
                int success = json.getInt(TAG_SUCCESS);

                if (success == 1) {

                    Log.d("", "Successfully logged session!");


                } else {
                    // failed 
                    Log.d("", "Not Successful!");
                }
            } catch (JSONException e) {
                e.printStackTrace();
            }
        }
    };

    thread.start();
}

/**
 * Background Async Task to Create new record
 * */
class LoginTask extends AsyncTask<String, String, String> {

    /**
     * Before starting background thread Show Progress Dialog
     * */
    protected void onPreExecute() {
        super.onPreExecute();
        pDialog = new ProgressDialog(Login.this);
        pDialog.setIcon(R.drawable.upload);
        pDialog.setTitle("Connecting to server");
        pDialog.setMessage("Validating login details..");
        pDialog.setIndeterminate(false);
        pDialog.setCancelable(true);
        pDialog.show();
    }

    /**
     * 
     * */
    protected String doInBackground(String... args) {

        strUsername = etUsername.getText().toString();
        strPassword = etPassword.getText().toString();

        // Building Parameters
        List<NameValuePair> params = new ArrayList<NameValuePair>();
        params.add(new BasicNameValuePair( TAG_USERNAME, strUsername ));
        params.add(new BasicNameValuePair( TAG_PASSWORD, strPassword ));

        // getting JSON Object
        // Note it accepts POST method only
        JSONObject json = jsonParser.makeHttpRequest(url_login,
            "POST", params);


        // check log cat from response
        Log.d("Create Response", json.toString());

        // check for success tag
        try {
            int success = json.getInt(TAG_SUCCESS);

            if (success == 1) {

                logSessionToServer(); // Save login to server

                saveUserInfoToDB(); // Save user info to DB

                saveUserloggedIn(); // Save user logged in to preference

                savePriestInfoToDB(); // Save priest to DB

                Log.d("", "Successful Login!");

                Intent i = new Intent(Login.this, MainActivity.class);
                startActivity(i);
                finish();
            } else {
                // failed 

                new Thread()
                {
                    public void run()
                    {
                        runOnUiThread(new Runnable()
                        {
                            public void run()
                            {
                                Toast.makeText(getBaseContext(), 
                                        "Not Successful Login", Toast.LENGTH_SHORT).show();
                            }
                        });
                    }

                }.start();

                Log.d("", "Not Successful Login!");
            }
        } catch (JSONException e) {
            e.printStackTrace();
        }

        return null;
    }

    /**
     * After completing background task Dismiss the progress dialog
     * **/
    protected void onPostExecute(String result) {
        // dismiss the dialog once done
        pDialog.dismiss();
    }

}

private void saveUserInfoToDB() {
    // TODO Auto-generated method stub


    Thread thread = new Thread(){
        public void run(){

            // Create the array 
            arraylist = new ArrayList<HashMap<String, String>>();
            // Retrieve JSON Objects from the given website URL in JSONfunctions.class
            String result = JSONFunctions.getJSONfromURL(url_view_loggedUser_profile);

            try {
                JSONArray jr = new JSONArray(result);
                for(int i=0;i<jr.length();i++)
                 {    
                          HashMap<String, String> map = new HashMap<String, String>();
                          jb = (JSONObject)jr.get(i);
                          map.put(TAG_FIRSTNAME, jb.getString(TAG_FIRSTNAME));
                          map.put(TAG_MIDDLENAME, jb.getString(TAG_MIDDLENAME));
                          map.put(TAG_LASTNAME, jb.getString(TAG_LASTNAME));
                          map.put(TAG_EMAIL, jb.getString(TAG_EMAIL));
                          map.put(TAG_AGE, jb.getString(TAG_AGE));
                          map.put(TAG_GENDER, jb.getString(TAG_GENDER));
                          map.put(TAG_USERNAME, jb.getString(TAG_USERNAME));
                          map.put(TAG_PASSWORD, jb.getString(TAG_PASSWORD));
                          map.put(TAG_BARANGAY, jb.getString(TAG_BARANGAY));
                          map.put(TAG_COMPLETEADDRESS, jb.getString(TAG_COMPLETEADDRESS));
                          map.put(TAG_CUS_ID, jb.getString(TAG_CUS_ID));
                          map.put(TAG_REG_DATE, jb.getString(TAG_REG_DATE));
                          map.put(TAG_BD_MONTH, jb.getString(TAG_BD_MONTH));
                          map.put(TAG_BD_DATE, jb.getString(TAG_BD_DATE));
                          map.put(TAG_BD_YEAR, jb.getString(TAG_BD_YEAR));
                          arraylist.add(map);

                          String strCusID = jb.getString(TAG_CUS_ID);
                          String strFname = jb.getString(TAG_FIRSTNAME);
                            String strMname = jb.getString(TAG_MIDDLENAME);
                            String strLname = jb.getString(TAG_LASTNAME);
                            String strEmail = jb.getString(TAG_EMAIL);
                            String strAge = jb.getString(TAG_AGE);
                            String strGender = jb.getString(TAG_GENDER);
                            String strUsername = jb.getString(TAG_USERNAME);
                            String strPassword = jb.getString(TAG_PASSWORD);
                            String strBarangay = jb.getString(TAG_BARANGAY);
                            String strCompleteAddress = jb.getString(TAG_COMPLETEADDRESS);
                            String strRegDate = jb.getString(TAG_REG_DATE);
                            String strBDMonth = jb.getString(TAG_BD_MONTH);
                            String strBDDate = jb.getString(TAG_BD_DATE);
                            String strBDYear = jb.getString(TAG_BD_YEAR);

                            try{



                                dbHelper.insertEntry(strCusID, strFname, strMname, strLname, strEmail, 
                                        strAge, strGender, strUsername, strPassword, 
                                        strBarangay, strCompleteAddress, strRegDate, 
                                        strBDMonth, strBDDate, strBDYear);
                                Log.d("Response", "Successfully inserted record");


                            } catch (SQLiteException e) {
                                Log.d("Response", "Not successful");
                            }

                 }
            } catch (JSONException e) {
                Log.e("Error", e.getMessage());
                e.printStackTrace();
            }
        }
    };

    thread.start();
}

private void savePriestInfoToDB() {
    // TODO Auto-generated method stub


    Thread thread = new Thread(){
        public void run(){

            // Create the array 
            arraylist = new ArrayList<HashMap<String, String>>();
            // Retrieve JSON Objects from the given website URL in JSONfunctions.class
            String result = JSONFunctions.getJSONfromURL(URL_VIEW_PRIESTS);

            try {
                JSONArray jr = new JSONArray(result);
                for(int i=0;i<jr.length();i++)
                 {    
                          HashMap<String, String> map = new HashMap<String, String>();
                          jb = (JSONObject)jr.get(i);
                          map.put(TAG_P_ID, jb.getString(TAG_P_ID));
                          map.put(TAG_P_FIRSTNAME, jb.getString(TAG_P_FIRSTNAME));
                          map.put(TAG_P_MIDDLENAME, jb.getString(TAG_P_MIDDLENAME));
                          map.put(TAG_P_LASTNAME, jb.getString(TAG_P_LASTNAME));
                          map.put(TAG_P_EMAIL, jb.getString(TAG_P_EMAIL));
                          map.put(TAG_P_AGE, jb.getString(TAG_P_AGE));
                          map.put(TAG_P_ADDRESS, jb.getString(TAG_P_ADDRESS));
                          map.put(TAG_P_CONTACT, jb.getString(TAG_P_CONTACT));
                          map.put(TAG_P_STATUS, jb.getString(TAG_P_STATUS));
                          arraylist.add(map);

                          String strP_ID = jb.getString(TAG_P_ID);
                          String strP_Fname = jb.getString(TAG_P_FIRSTNAME);
                            String strP_Mname = jb.getString(TAG_P_MIDDLENAME);
                            String strP_Lname = jb.getString(TAG_P_LASTNAME);
                            String strP_Email = jb.getString(TAG_P_EMAIL);
                            String strP_Age = jb.getString(TAG_P_AGE);
                            String strP_Address = jb.getString(TAG_P_ADDRESS);
                            String strP_Status = jb.getString(TAG_P_STATUS);
                            String strP_Contact = jb.getString(TAG_P_CONTACT);

                            try{


                                dbHelper.insertPriest(strP_ID, strP_Fname, strP_Mname, 
                                        strP_Lname, strP_Email, strP_Age, 
                                        strP_Address, strP_Status, strP_Contact);
                                Log.d("Response - Priest", "Successfully inserted record");


                            } catch (SQLiteException e) {
                                Log.d("Response - Priest", "Not successful");
                            }

                 }
            } catch (JSONException e) {
                Log.e("Error", e.getMessage());
                e.printStackTrace();
            }
        }
    };

    thread.start();
}

Answer 1

Have descriptive, intuitive variable names. (Example: what is etUsername? No clue.)

You have a List of a NameValuePair. It looks like the your custom NameValuePair class is much like a key-value pair in a Map. I think it would simplify your code a lot if you just went ahead and used HashMap<String, String> instead of jumping through needless hoops.

Be more consistent in your naming conventions. You have some variables which are named in camelCase (which is the accepted way to do it in Java), and then you have some which have the elements separated by underscores (url_log_session). The capitalized/underscore format is good for constants (like your TAG_USERNAME), but that's it.

It's good that you're trying to comment your code, but some of your comments are overkill and unnecessary. Because you chose good names for your methods, the code is self-documenting in some places, which is a good thing! So the following comments are pretty much useless, as they say exactly what the method names express:

saveUserInfoToDB(); // Save user info to DB
saveUserloggedIn(); // Save user logged in to preference
savePriestInfoToDB(); // Save priest to DB

---

When dealing with Threads, you should almost always have the run() functionality defined in some class that implements Runnable rather than extending Thread itself. So you can have an anonymous class or an explicit one. The standard way to do it in-line like you tried to is like this:

Thread thread = new Thread(new Runnable() {
    @Override
    public void run() {
        // do stuff
    }
});

If you don't need the reference to the thread, you can even do it more simply, like the following:

new Thread(new Runnable() {
    @Override
    public void run() {
        // do stuff
    }
}).start();

Answer 2

There is still a bunch of // TODO Auto-generated method stub in your code. Those are generated by the IDE and should be remove since it's just noise.

You still have a bunch of e.printStackTrace(); in your some of your catch clause. Those should be replace by a proper logger of the android platform, since as you can read in this question it simply write to a general log file. As per Simon's comments, you should make sure you're using a tag when you log. This will help filtering the log file. Your logging statement will be like this :

Log.d(MEANINGFUL_TAG, "The message to log");

Be carefull with your indentation/white-space style, this look really weird :

          if (success == 1) {

               Log.d("", "Successfully logged session!");


           } else {
               // failed 
               Log.d("", "Not Successful!");
           }

I'm not an knowledgeable enough in android to see that it's wrong but protected String doInBackground(String... args) should probably be void since you're only returning null at the end of the method. If you don't need to return something, then don't define a return type.

Answer 3

Threads

Your application seem to be both using Thread and AsyncTask. For Android, I'd recommend using AsyncTasks and/or ExecutorServices. That is, avoid using Thread directly. This is just my opinion though.

---

JSON

This code looks bananas bad:

map.put(TAG_FIRSTNAME, jb.getString(TAG_FIRSTNAME));
map.put(TAG_MIDDLENAME, jb.getString(TAG_MIDDLENAME));
map.put(TAG_LASTNAME, jb.getString(TAG_LASTNAME));
map.put(TAG_EMAIL, jb.getString(TAG_EMAIL));
(...)

One step to clean it up would be to use a String array of all the values you want, and then loop through that array.

String[] keys = { TAG_FIRSTNAME, TAG_MIDDLENAME, TAG_LASTNAME, TAG_EMAIL };
for (String key : keys) {
    map.put(key, jb.getString(key));
}

However, an even better way to do it would be to use the Jackson library which can parse the JSON into a Java object. I can highly recommend this library, I have used it in multiple projects (also for an Android application) and it works great! Take a look at Jackson in five minutes to understand the basics of using that library.