2
\$\begingroup\$

I created a small Android project with an AsyncTask. It simulates a time consuming task.

Please review my code and tell me what you would do on a different way and why. Comments not specific to Android but general software development are welcome.

CounterTask.java:

package my.tasks;

import java.util.Random;

import android.app.AlertDialog;
import android.app.AlertDialog.Builder;
import android.app.Dialog;
import android.app.ProgressDialog;
import android.os.AsyncTask;
import android.util.Log;

/**
 * @author 2
 * 
 *         Simulates a time consuming task, counts in the background from 0 to
 *         the given value in first parameter. Its progress is published via an
 *         integer which is a percentual value 0..100. The task may not
 *         complete, it may fail or it may be cancelled by the user. In case of
 *         success it returns true.
 */
public class CounterTask extends AsyncTask<Integer, Integer, Boolean> {

    private static final long DELAY = 100;
    private static final int PROBABILITY_TO_FAIL_EVERY_STEP = 1;
    private static final String TAG = "CounterTask";
    private ProgressDialog progressDialog;
    private Random random;

    /**
    * @param progressDialog
    * a ProgressDialog the task can use to display its progress.
    */
    public CounterTask(ProgressDialog progressDialog) {
        this.progressDialog = progressDialog;
        this.progressDialog.setMax(100);
        this.random = new Random();
    }

    /*
    * (non-Javadoc)
    * 
    * @see android.os.AsyncTask#doInBackground(Params[])
    * 
    * Set params[0] to the maximum value you want the task to count to.
    */
    @Override
    protected Boolean doInBackground(Integer... params) {
        validateParams(params);
        int count_max = params[0];

        for (int i = 0; i < count_max; i++) {
            int progressPercent = calculateProgressPercent(i, count_max);
            publishProgress(progressPercent);
            Log.i(TAG, "Counter: " + Integer.toString(i));
            Log.i(TAG, "Progress published: " + Integer.toString(progressPercent) + "%");
            if (sleep() == false) {
                return false;
            }
            if (doesFail(PROBABILITY_TO_FAIL_EVERY_STEP)) {
                Log.i(TAG, "Background fails.");
                return false;
            }
        }

        Log.i(TAG, "Background succeeds.");
        return true;
    }

    @Override
    protected void onCancelled() {
        super.onCancelled();
        Log.i(TAG, "Background cancelled.");
    }

    @Override
    protected void onPostExecute(Boolean result) {
        super.onPostExecute(result);
        progressDialog.dismiss();

        String message = null;
        if (result)
            message = "Background succeeded.";
        else
            message = "Background failed.";
        Log.i(TAG, message);
        createAlertDialog(message).show();
    }

    private Dialog createAlertDialog(String message) {
        AlertDialog.Builder builder = new Builder(progressDialog.getContext());
        AlertDialog dialog = builder.setMessage(message).setTitle("Result").setCancelable(true).create();
        return dialog;
    }

    @Override
    protected void onProgressUpdate(Integer... values) {
        super.onProgressUpdate(values);
        progressDialog.setProgress(values[0]);
    }

    /**
    * Helper method to decide if the task fails in this step.
    * 
    * @param probabilityToFail
    *            Value 0..100 the probability to fail.
    * @return true if it fails, false if not
    */
    private boolean doesFail(int probabilityToFail) {

        int prob = random.nextInt(100);
        if (prob < probabilityToFail)
            return true;
        else
            return false;
    }

    /**
    * The thread sleeps from step to step to simulate a time consuming task.
    * 
    * @return
    */
    private boolean sleep() {
        try {
            Thread.sleep(DELAY);
        } catch (InterruptedException e) {
            return false;
        }
        return true;
    }

    private int calculateProgressPercent(int done, int max) {
        done = done * 100;
        return done / max;
    }

    private void validateParams(Integer... integers) {
        if (integers == null || integers.length != 1 || integers[0] < 0)
        throw new IllegalArgumentException();
    }
}

MainActivity.java:

package my.application;

import my.tasks.CounterTask;
import android.app.Activity;
import android.app.ProgressDialog;
import android.content.DialogInterface;
import android.content.DialogInterface.OnCancelListener;
import android.os.Bundle;
import android.view.View;
import android.view.View.OnClickListener;
import android.widget.Button;

public class MainActivity extends Activity implements OnClickListener, OnCancelListener {
    private CounterTask counterTask;

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);

        Button startButton = (Button) findViewById(R.id.startButton);
        startButton.setOnClickListener(this);
    }

    @Override
    public void onClick(View view) {
        int id = view.getId();

        switch (id) {
        case R.id.startButton:
            ProgressDialog progressDialog = createProgressDialog();
            progressDialog.show();
            CounterTask ct = new CounterTask(progressDialog);
            counterTask = (CounterTask) ct.execute(100);
            break;

        default:
            break;
        }
    }

    private ProgressDialog createProgressDialog() {

        ProgressDialog progressDialog = new ProgressDialog(this);
        progressDialog.setProgress(0);
        progressDialog.setMax(100);
        progressDialog.setTitle("Progress");
        progressDialog.setCancelable(true);
        progressDialog.setOnCancelListener(this);
        progressDialog.setProgressStyle(ProgressDialog.STYLE_HORIZONTAL);
        return progressDialog;
    }

    @Override
    public void onCancel(DialogInterface arg0) {
        counterTask.cancel(true);
    }
}
\$\endgroup\$
3
\$\begingroup\$

Looks good. You could put the progressDialog creation and show in onPreExecute: http://developer.android.com/reference/android/os/AsyncTask.html#onPreExecute() . It will run in the UI thread and will encapsulate more of the dialog logic.

You can cut out a few lines here

String message = null;
if (result)
    message = "Background succeeded.";
else
    message = "Background failed.";

becomes

String message = result ? "Background succeeded." : "Background failed";
\$\endgroup\$
1
\$\begingroup\$

Conditional operator can be used to improve the following method:

private boolean doesFail(int probabilityToFail) {

    int prob = random.nextInt(100);
    if (prob < probabilityToFail)
        return true;
    else
        return false;
}

Can be improved as

private boolean doesFail(int probabilityToFail) {
    return (random.nextInt(100) < probabilityToFail);
}
\$\endgroup\$
0
\$\begingroup\$
  • do not pass ProgressDialog as parameter in constructor (what will happend when activity will be recreated and task is still running or finished?) read more about SERIALIZATION
  • don't use keyword this in constructor if u not calling other constructor involving creation of its object or using it as a qualifier when referencing fields
  • use onPreExecute() to show ProgressDialog like this:

    @Override
    protected void onPreExecute() {
         /** show progress dialog */
        showProgressDialog(_context,_message);
    }
    
    /** show pd method */
    private void showProgressDialog(Context context, String message) {
        /** check if progress dialog is not already created or if context didn't changed - eg. during screen rotation */
        if(_progressDialog == null || _progressDialog.getContext().equals(context)) {
            /** create progress dialog */
            _progressDialog = new ProgressDialog(context);
            _progressDialog.setCancelable(false);
        }
        /** set message */
        _progressDialog.setMessage(message);
        /** show pd */
        _progressDialog.show();
    }
    
  • always find places when your task could be terminatet by you or other factors and try dismiss progress dialog

    private void dismissProgressDialog() {
        try {
            /** check if  progress dialog is showing */
            if (_progressDialog != null && _progressDialog.isShowing()) {
                /** dismiss login dialog */
                _progressDialog.dismiss();
            }
        } catch (Exception e) {
            // Handle or log or ignore
        } finally {
            _progressDialog = null;
        }
    }
    
    • onPostExecute you should dismiss progress dialog not showing it

    • to show your result create a callback to Activity

define interface:

public interface AsyncTaskResultListener<T> {

    /**  notifies Activity when task is done.
     *   @return true if result parsed OK
     *   other way false to repeat with RepeatTask()*/
    boolean onTaskComplete(T result);
    void repeatTask();
}

in async task:

    AsyncTaskResultListener<T> _callback;
    Context _context;

    MyAsyncTask(Context context) {
       /** check for valid context */
        if(Activity.class.isAssignableFrom(context.getClass()) {
            _contex = context;
            /** check for valid callback */
             if(AsyncTaskResultListener.class.isAssignableFrom(context.getClass()) {
                 /** cast it to interface */
                 _callback = (AsyncTaskResultListener<T>) context;
            }
        }
    }

    @Override
    protected void onPostExecute(T result) {
       if(_callback!=null) {
            if(!callback.onTaskComplete(result) {
                /** repeat task */
                callback.repeatTask()
            }
        }
        /** dismiss pd */
    }

in activity:

public MyActivity exrtends Activity implements AsyncTaskResultListener<T> {

    @Override 
    boolean onTaskComplete(T result) {
        /** do work with your result 
        if satisfied return true, to repeat task return false */
        return true;
    }

    @Override
    public void repeatTask() {
        /** free to make decision what to do */
    }
}

i got little more to point out but my hands have enough

\$\endgroup\$

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.