Android Thread and Runnable

Question

This is my code for running a background thread. I believe it is very poor in naming and code structure.

package com.ocs.socialshare.bloggershare;
import android.app.Activity;
import android.os.Bundle;
import android.view.Menu;
import android.view.View;
import android.view.View.OnClickListener;
import android.widget.Button;
import android.widget.EditText;
import com.google.gdata.util.AuthenticationException;
public class MainActivity extends Activity implements OnClickListener{
 private Thread doinBackground;
 private EditText username,password,blogurl;
 private Button login;
 private String user,pass,bloglink;
 @Override
 protected void onCreate(Bundle savedInstanceState) {
 super.onCreate(savedInstanceState);
 setContentView(R.layout.activity_main);
 findViewById();
 setOnclickListenerRegister();
 }
 private void findViewById() {
 // TODO Auto-generated method stub
 username = (EditText) findViewById(R.id.username);
 password = (EditText) findViewById(R.id.password);
 blogurl = (EditText) findViewById(R.id.blogurl);
 login = (Button) findViewById(R.id.login);
 }
 private void setOnclickListenerRegister() {
 // TODO Auto-generated method stub
 login.setOnClickListener(this);
 }
 private void blogShare(final String user,final String pass,final String bloglink) {
 // TODO Auto-generated method stub
 doinBackground =new Thread(new Runnable() 
 {
 @Override
 public void run() 
 {
 // TODO Auto-generated method stub
 BloggerClient blog = new BloggerClient(getApplicationContext());
 try {
 blog.oAuthBlogger(user,pass,bloglink);
 } 
 catch (AuthenticationException e) 
 {
 // TODO Auto-generated catch block
 e.printStackTrace();
 }
 }
 });
 doinBackground.start();
 }
 @Override
 public boolean onCreateOptionsMenu(Menu menu) {
 // Inflate the menu; this adds items to the action bar if it is present.
 getMenuInflater().inflate(R.menu.main, menu);
 return true;
 }
 @Override
 public void onClick(View view) {
 // TODO Auto-generated method stub
 int viewid = view.getId();
 if(viewid == R.id.login)
 {
 validateFeild();
 blogShare(user,pass,bloglink);
 }
 }
 private void validateFeild() {
 // TODO Auto-generated method stub
 user = username.getText().toString();
 pass = password.getText().toString();
 bloglink = blogurl.getText().toString();
 }
}

Answer 1

The first thing I would say is to use an AsyncTask here instead of normal Thread/Runnable. AsyncTask helps you to work better with the thread and UI thread better.

It's a login system, so disable login buttons while the application is login to avoid the user to press multiple times on Login.

http://developer.android.com/reference/android/os/AsyncTask.html

• prasad thangavel: I read one blog asynctask is produce problem with orientation changes. is that true?
• Marco Acierno: AsyncTask could give problems sometimes (never had problem by myself (except the activity killed before async finish). my login system uses asynctask) but could happen the asynctask is executed and the activity not exist anymore or some cases similar (too long for a comment, for me) but you could do some checks and avoid some problems. Anyway you could write something like a Service (they don't spawn a new thread! ) and put logic here then say back response to activity (but its pretty the same thing)

Answer 2

• Please remove all // TODO Auto-generated method stub comments. Your methods are implemented. You don't need those comments any more.

• Indentation. If you're using Eclipse, select all code and press Ctrl + Shift + I to ident your code properly. If you're using NetBeans, press Alt + Shift + F. Your try-catch inside the blogShare method is incorrectly indented.

• The correct spelling is validateField

• user, pass and bloglink are only set within validateField and only read directly after that method invocation. Get rid of the validateField method (which currently doesn't do any actual validation so it's a poorly named method). Use local variables within the onClick method.

• int viewid = view.getId(); is IMO not needed to set as a variable, as you only check it once, use view.getId() directly.

• setOnclickListenerRegister is only called once from onCreate and this method only contains one line, so what you have done is to extract one statement to it's own method and then call that method which were only called once. This extraction is not worth it. Remove this method and put login.setOnClickListener(this); back to the onCreate method where it belongs.

• I totally agree with Marco that you should use AsyncTask. If you encounter any problems with orientation changes, deal with those correctly. Don't avoid using AsyncTask just because you're afraid that it will cause problems.

• Marc-Andre: I totally with Marco should probably be I totally agree.
• Simon Forsberg: @Marc-Andre Fixed, thanks for reviewing my review!