Multiple background tasks

Question 1

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();
}

Question 2

To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question

Question 3

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();

Question 4

I'm not familiar with Thread but isn't an old way to work directly with the Thread class ? Don't we need to submit a Runnable to an Executor or something close to that ? I'm asking honestly I don't really have a clue here. (also you have weird dot in you answer is it normal ?)

Question 5

@Marc-Andre I added the dots to the answer because otherwise the code formatting doesn't work with the bullet points. Maybe I'll reformat it somehow. And yeah, you can create a subclass of Thread, but it's considered poor object-oriented design. A subclass should be providing additional functionality to a class. When you subclass Thread simply to supply the run() method, you're not actually extending the Thread class's behavior. See this SO Q&A for more detailed explanations

Question 6

And you are kind of submitting the Runnable to an executor. The executor is the Thread. :)

Question 7

@Marc-Andre Certainly, you could do something like that. I'm not sure why you would want to, but you could. I've really only ever used Thread and sometimes a Future for Callables. I'm afraid I haven't used the ExecutorService API to be able to even discuss it.

Question 8

@Marc-Andre I agree with the ExecutorService suggestion but have a minor nit-pick: It's supposed to be Executors.newSingleThreadExecutor() :)

Question 9

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.

Question 10

One more thing to add about logging: It's good to use a proper tag for the logging, to make it easy to filter the log when using adb logcat

Question 11

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.

asteri asteri 4,9695 gold badges25 silver badges48 bronze badges · Answer 1 · 2014-05-23 13:42:21Z

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();

I'm not familiar with Thread but isn't an old way to work directly with the Thread class ? Don't we need to submit a Runnable to an Executor or something close to that ? I'm asking honestly I don't really have a clue here. (also you have weird dot in you answer is it normal ?)
@Marc-Andre I added the dots to the answer because otherwise the code formatting doesn't work with the bullet points. Maybe I'll reformat it somehow. And yeah, you can create a subclass of Thread, but it's considered poor object-oriented design. A subclass should be providing additional functionality to a class. When you subclass Thread simply to supply the run() method, you're not actually extending the Thread class's behavior. See this SO Q&A for more detailed explanations
And you are kind of submitting the Runnable to an executor. The executor is the Thread. :)
@Marc-Andre Certainly, you could do something like that. I'm not sure why you would want to, but you could. I've really only ever used Thread and sometimes a Future for Callables. I'm afraid I haven't used the ExecutorService API to be able to even discuss it.
@Marc-Andre I agree with the ExecutorService suggestion but have a minor nit-pick: It's supposed to be Executors.newSingleThreadExecutor() :)

Marc-Andre Marc-Andre 6,7795 gold badges39 silver badges65 bronze badges · Answer 2 · 2014-05-23 14:15:42Z

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.

One more thing to add about logging: It's good to use a proper tag for the logging, to make it easy to filter the log when using adb logcat

score 3 · Answer 3 · 2014-05-23 14:14:05Z

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.

Stack Exchange Network

Multiple background tasks

3 Answers 3

Threads

JSON

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Multiple background tasks

3 Answers 3

Threads

JSON

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions