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.

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();
}
share|improve this 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 –  Simon André Forsberg May 23 at 14:03

3 Answers 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();
share|improve this answer
    
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 May 23 at 13:46
    
@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 –  asteri May 23 at 13:49
    
And you are kind of submitting the Runnable to an executor. The executor is the Thread. :) –  asteri May 23 at 13:50
1  
@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. –  asteri May 23 at 13:56
1  
@Marc-Andre I agree with the ExecutorService suggestion but have a minor nit-pick: It's supposed to be Executors.newSingleThreadExecutor() :) –  Simon André Forsberg May 23 at 14:49

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.

share|improve this answer
1  
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 –  Simon André Forsberg May 23 at 14:51

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.

share|improve this answer

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.