Alternative class to imitate AsyncTask class (deprecated) with Thread and Handler classes

Question

As the title says, I have created an alternative class to imitate AsyncTask class (deprecated) with Thread and Handler classes but i feel uncomfortable to use it.(the app works fine with my class) Can you guys tell me is it ok or not? I feel like there is nothing changed. The reason i want to stop using AsyncTask is memoryleak issue.

import android.os.Handler;

public class BackgroundTask
{
    protected Handler localHandler;
    protected Thread localThread;

    public void execute()
    {
        this.localHandler = new Handler();
        this.onPreExecute(); //onPreExecute Method

        this.localThread = new Thread(new Runnable()
        {
            @Override
            public void run()
            {
                BackgroundTask.this.doInBackground(); //doInBackground Method
                BackgroundTask.this.localHandler.post(new Runnable()
                {
                    @Override
                    public void run()
                    {
                        BackgroundTask.this.onPostExecute(); //onPostExecute Method
                    }
                });
            }
        });
        this.localThread.start();
    }

    public void cancel()
    {
        if(this.localThread.isAlive())
            this.localThread.interrupt();
    }

    protected void onPreExecute()
    {

    }

    protected void doInBackground()
    {

    }

    protected void onPostExecute()
    {

    }
}

Example for use (I can interrupt the Thread when i want so this must be solving the leak issue but i want this code to be seen by some other programmers too.)

public class HttpPostUserScore extends BackgroundTask
{
    private final Context context;
    private Exception taskException;
    private final String point;

    public HttpPostUserScore(Context context, String point)
    {
        this.context = context;
        this.point = point;
    }

    @Override
    protected void doInBackground()
    {
        try
        {
            OkHttpClient client = new OkHttpClient();

            FormBody.Builder formBuilder = new FormBody.Builder();
            formBuilder.add("token", User.loggedUser.getToken());
            formBuilder.add("score", this.point);
            RequestBody requestBody = formBuilder.build();

            Request request = new Request.Builder().url(this.context.getString(R.string.user_score_api_url_body)).post(requestBody).build();
            Response response = client.newCall(request).execute();

            Log.println(Log.WARN, "ScorePost Response", response.body().string());
        }
        catch (Exception e) { this.taskException = e; }
    }

    @Override
    protected void onPostExecute()
    {
        if(this.taskException != null)
            Log.println(Log.ERROR, "ScorePost Error", this.taskException.getMessage());
        else
            Toast.makeText(this.context, this.point + " Points Added!", Toast.LENGTH_LONG).show();
    }
}

I have examined some codes from Facebook's android login api. They are still using AsyncTask for asynchronous programming. Is it a big deal or not?

Answer 1

    public void execute()
    {
        this.localHandler = new Handler();
        this.onPreExecute(); //onPreExecute Method

        this.localThread = new Thread(new Runnable()
        {
            @Override
            public void run()
            {
                BackgroundTask.this.doInBackground(); //doInBackground Method
                BackgroundTask.this.localHandler.post(new Runnable()
                {
                    @Override
                    public void run()
                    {
                        BackgroundTask.this.onPostExecute(); //onPostExecute Method
                    }
                });
            }
        });
        this.localThread.start();
    }

This part would be much easier to read using method references:

public void execute()
{
    localHandler = new Handler();
    onPreExecute();

    localThread = new Thread(this::backgroundExecutor)
    localThread.start();
}

protected void backgroundExecutor() {
    doInBackground();
    
    localHandler.post(this::onPostExecute)
}

---

        this.localHandler = new Handler();
        this.onPreExecute(); //onPreExecute Method

One should only use this if it is necessary to differentiate between a local and a member variable. And that should only be necessary in setters and the constructor. You can remove this and remove a lot of noise with it.

---

        this.localHandler = new Handler();
        this.onPreExecute(); //onPreExecute Method

Comment on why you're doing something, not what you're doing. This comment tells me nothing the code on the left doesn't tell me.

---

        if(this.localThread.isAlive())
            this.localThread.interrupt();

One should only use curly-braces to make the scopes easily visible and minimize possible scope errors.

---

Your code has a few problems when it comes to threading. Invoking execute multiple times will reset the local state and will yield inconsistent and potentially unknown results, as localHandler and localThread are getting overwritten with every call (imagine execute being called from multiple threads). That will also impact cancel, as it will only be called on the last run thread. Additionally, calling interrupt might or might not actually end the thread. There is no failsafe way to end a thread except killing the JVM.

Your implementation has similar issues, as it shares state using class members, which means that multiple invocations will lead to lost state and possible confusions (three invocations, the last two fail, but the first one is the last to finish).

What you're trying to do is not easy. I'd recommend looking at the implementation of the AsyncHandler class and getting ideas what they were doing. Depending in your use-case, for example if you have a lot of background actions, you might want instead to utilize a thread-pool, to minimize constantly spinning up threads.