7
\$\begingroup\$

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

    }

}
\$\endgroup\$
6
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ I read one blog asynctask is produce problem with orientation changes. is that true? \$\endgroup\$ – prasad thangavel Mar 12 '14 at 6:34
  • \$\begingroup\$ 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) \$\endgroup\$ – Marco Acierno Mar 12 '14 at 6:45
8
\$\begingroup\$
  • 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I totally with Marco should probably be I totally agree. \$\endgroup\$ – Marc-Andre Mar 12 '14 at 18:31
  • \$\begingroup\$ @Marc-Andre Fixed, thanks for reviewing my review! \$\endgroup\$ – Simon Forsberg Mar 12 '14 at 18:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.