11
\$\begingroup\$

This is the first time that I have played around with AsyncTask in Android and I wanted to make sure I'm using it correctly. The idea is I'm grabbing all the rows from a table in the database using dao and putting them into an Arraylist based off my Item class. Once finished, I added it to a custom Adapter and add it to my ListView. I wasn't sure if the placement to the AsyncTask class inside the ListFragment is correct or if I used getActivity() correctly ( would it have been better to pass it as a variable?). Any other constructive criticism is welcome as well.

package com.domain.myapp;

import java.util.ArrayList;

import android.app.Activity;
import android.os.AsyncTask;
import android.os.Bundle;
import android.support.v4.app.ListFragment;
import android.view.Menu;
import android.view.MenuInflater;
import android.view.MenuItem;
import android.view.View;
import android.widget.ListView;

import com.domain.myapp.adapters.ItemsListAdapter;
import com.domain.myapp.database.DAO;
import com.domain.myapp.models.Item;

public class ItemsListFragment extends ListFragment {

    private ItemsListAdapter falAdp;
    private OnFragmentInteractionListener mListener;

    public ItemsListFragment() {
    }

    @Override
    public void onCreateOptionsMenu(Menu menu, MenuInflater inflater) {

        inflater.inflate(R.menu.item_list_view_menu, menu);
    }

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        getItemLists gfl = new getItemLists();
        gfl.execute();
    }

    @Override
    public void onAttach(Activity activity) {
        super.onAttach(activity);
        try {
            mListener = (OnFragmentInteractionListener) activity;
        } catch (ClassCastException e) {
            throw new ClassCastException(activity.toString()
                    + " must implement OnFragmentInteractionListener");
        }
    }

    @Override
    public void onDetach() {
        super.onDetach();
        mListener = null;
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        mListener.onFragmentInteraction(item.getItemId(), 0);
        return true;
    }

    @Override
    public void onListItemClick(ListView l, View v, int position, long id) {
        super.onListItemClick(l, v, position, id);

        if (null != mListener) {
            mListener.onFragmentInteraction(R.id.ViewItemDetails, falAdp
                    .getItem(position).getId());
        }
    }

    public interface OnFragmentInteractionListener {
        public void onFragmentInteraction(int mnuId, int Id);
    }

    private class getItemLists extends
            AsyncTask<Void, String, ArrayList<Item>> {
        @Override
        protected void onPreExecute() {
            super.onPreExecute();
        }

        @Override
        protected void onProgressUpdate(String... values) {
            super.onProgressUpdate(values);
        }

        @Override
        protected ArrayList<Item> doInBackground(Void... params) {
            DAO dao = new DAO();
            ArrayList<Item> fal = dao.ItemsGetList(getActivity());
            return fal;
        }

        @Override
        protected void onPostExecute(ArrayList<Item> result) {
            super.onPostExecute(result);

            falAdp = new ItemsListAdapter(getActivity(),
                    R.layout.fragment_list_item_text_view, result);
            setListAdapter(falAdp);
        }
    }
}
\$\endgroup\$
8
\$\begingroup\$

All in all, this code is great. I don't see much in the way of problems. There are a few nit-picks:

    } catch (ClassCastException e) {
        throw new ClassCastException(activity.toString()
                + " must implement OnFragmentInteractionListener");
    }

The above code takes one ClassCastException and swaps it with another. Is it necessary? If it is necessary (because the current message does not give details on the current activity), then I recommend initializing the cause of the thrown CCE:

    } catch (ClassCastException e) {
        ClassCastException tothrow = new ClassCastException(activity.toString()
                + " must implement OnFragmentInteractionListener");
        tothrow.initCause(e);
        throw tothrow;
    }

it is a pain, but you benefit from getting all the information in the stack trace. Currently you are losing some.

The only other issue I see is the extension of the AsyncTask. You have re-implemented the methods:

    @Override
    protected void onPreExecute() {
        super.onPreExecute();
    }

    @Override
    protected void onProgressUpdate(String... values) {
        super.onProgressUpdate(values);
    }

This is completely unnecessary and you can delete that code. Just let the ancestor code be.

\$\endgroup\$
  • 1
    \$\begingroup\$ Cool, thank you for your input. I will make the changes to my code. \$\endgroup\$ – Chillie Mar 20 '14 at 12:58
2
\$\begingroup\$

A minor point about your Java code. For the parameter types for getItemLists and onPostExecute(), you should use the interface List rather than the class ArrayList. Similarly,

ArrayList<Item> fal = dao.ItemsGetList(getActivity());

should be

List<Item> fal = dao.ItemsGetList(getActivity());

This will make your code more flexible in case the type of list later needs to be changed.

\$\endgroup\$
0
\$\begingroup\$

Few minor things;

I would consider renaming your member variables to something like the following;

private ItemsListAdapter falAdp; => mItemsAdapter;
private OnFragmentInteractionListener mListener; => mInteractionListener;

Follows the android m prefix, and makes it slightly more readable.

Also your getItemLists async task, should be renamed to something like the following;

getItemLists = > RetrieveItemListTask

Should begin with an uppercase letter and the class name should reflect what it actually is.

You could also consider moving your task execute into onResume, this is called just after onCreate. This would then update your list when your fragment is being resumed and not only on creating the initial instance.

@Override
public void onResume() {
    super.onResume();
    mRetrieveItemListTask.execute();
}

Finally you do not need to recreate your adapter every time your async task runs, you can just reset the data and call notifydatasetchanged.

For example, create your adapter in onCreate:

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    mItemsAdapter = new ItemsListAdapter(getActivity(), R.layout.fragment_list_item_text_view);
    setListAdapter(mItemsAdapter);
}

then;

@Override
protected void onPostExecute(ArrayList<Item> result) {
    super.onPostExecute(result);

    mItemsAdapter.clearAll();
    mItemsAdapter.addAll(result); //which will call notifydatasetchanged
}

Nothing wrong with your listener implementation. But might be worth having a look at an EventBus like otto, make it much cleaner :D

\$\endgroup\$

protected by Jamal Jul 8 '17 at 6:16

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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