Asynchronous login form handler

Question

I have an app that user submit the log in form , when it sent the data to server app create a connection for its account.

In this connection i have an integer field named as state.
the state value is : 1 for connecting, 2 for connected and 0 for failed.

I show a dialog to user display Connecting ... and then check the state of connection. if its return 0 or 2 dismiss the dialog and show the related message else if it doesn't change after 15 sec dismiss dialog and change the state to 0 ! I do the logic in this way :

final AsyncTask<IImConnection, Void, Boolean> task = new CheckSignInTask();
timeOut = new TimeOut(task);
handler.postDelayed(timeOut, 60 * 1000);
task.execute(conn);

CheckSignInTask class :

 private class CheckSignInTask extends AsyncTask<IImConnection, Void, Boolean> {

        private IImConnection connection;

        @Override
        protected Boolean doInBackground(IImConnection... params) {
            try {
                connection = params[0];
                if (connection != null){
                    while (connection.getState() == ImConnection.LOGGING_IN) {
                        // do nothing while connection state not change

                    }
                    return true;
                }
                else{
                    return false;
                }

            } catch (RemoteException e) {
                e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
            }
            return true;  //To change body of implemented methods use File | Settings | File Templates.
        }

        protected void onPreExecute() {
            dialog = new ProgressDialog(AccountActivity.this);
            dialog.setMessage("Signing in...");
            dialog.show();
        }

        protected void onPostExecute(final Boolean result) {
            if (dialog.isShowing()) {
                dialog.dismiss();
            }
            if (result) {
                try {
                    if (connection.getState() == ImConnection.LOGGED_IN) {
                        isSignedIn = true;
                        setResult(RESULT_OK);
                        finish();
                    } else {
                        isSignedIn = false;
                        deleteAccount();
                        displayLoginError("Login failed ! maybe your username/password is incorrect");
                    }
                } catch (RemoteException e) {
                    e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
                }
            }
            else{
                recreate();
            }
        }

        @Override
        protected void onCancelled() {
            if (dialog.isShowing()) {
                dialog.dismiss();
            }
            super.onCancelled();
        }
    }

TimeOut class :

 public class TimeOut implements Runnable {
        private AsyncTask task;

        public TimeOut(AsyncTask task) {
            this.task = task;
        }

        @Override
        public void run() {
            if (task.getStatus() == AsyncTask.Status.RUNNING) {
                task.cancel(true);
                deleteAccount();
                try {
                    if (dialog != null && dialog.isShowing()) {
                        dialog.dismiss();
                    }
                } catch (java.lang.IllegalArgumentException iae) {
                    //dialog may not be attached to window if Activity was closed
                }
                displayLoginError("Login failed ! check your internet connetction or maybe your username/password is incorrect");
            }
        }
    }

Its work ! but im not sure is it a good approach ?

VoiceOfUnreason · Accepted Answer · 2015-02-16 16:14:38Z

Disclaimer: not an android developer....

Thoughts on the Problem

A few thoughts, right off the top.

In this connection i have an integer field named as state. the state value is : 1 for connecting, 2 for connected and 0 for failed

First - that should probably be implemented as an enum, rather than as an integer

Second - that suggests that you are trying to implement a state machine (unsurprising; you are trying to implement a login protocol, and protocols almost always demand state machines). I'd encourage you to refactor the code and make the state machine explicit; you might want to consider stateless4j as part of the implementation.

Third - doInBackground() needs to be checking for cancellation. Normally, I would expect that check to be the condition in the while loop.

while (! isCancelled()) {
    if (connection.getState() == ImConnection.LOGGING_IN) {
        ...
    }
}

With the while loop written the way that you have it, it looks as though you are busy spinning while waiting for the state to change. I'm not keen on that at all; Thread.sleep(), SystemClock.sleep(), something to free up CPU to do the work. I'd probably go with Thread.sleep() myself, since I would want this worker thread to respond quickly to the cancel() command (which almost certainly interrupts() the running thread).

It's not really clear to me that you need the cancellation to be running on a timer separate from the CheckSignInTask - why not have the task check the time out itself? (If because Reasons[tm], those reasons should be clearly documented in the code so that the next coder to come along doesn't get too clever).

Style notes

What's the point of assigning a member variable if you are only using it in one place?

        protected Boolean doInBackground(IImConnection... params) {
        try {
            IImConnection connection = params[0];

Also, it's disconcerting that you are passed an array of connections, but you only ever manipulate the first one?

public class TimeOut implements Runnable {
        private AsyncTask task;

        public TimeOut(AsyncTask task) {
            this.task = task;
        }

You planning on changing what task you are timing out later? Of course not. So make that explicit

public class TimeOut implements Runnable {
        private final AsyncTask task;
        ...

Try to avoid magic numbers

handler.postDelayed(timeOut, 60 * 1000);

Is that a minute? A thousand hours? 60 millenia?

handler.postDelayed(timeOut, TimeUnit.MINUTES.toMillis(1));

or even better

final long LOGIN_PROTOCOL_TIMEOUT = TimeUnit.MINUTES.toMillis(1);
handler.postDelayed(timeOut, LOGIN_PROTOCOL_TIMEOUT);

Stack Exchange Network

Asynchronous login form handler

1 Answer 1

Thoughts on the Problem

Style notes

You must log in to answer this question.

Hot Network Questions

Asynchronous login form handler

1 Answer 1

Thoughts on the Problem

Style notes

You must log in to answer this question.

Related

Hot Network Questions