Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am implementing a thread like the following. Is my implementation correct?

It works well, but I need a check.

myList.setOnItemClickListener(new OnItemClickListener() {

    @Override
    public void onItemClick(AdapterView<?> parent, final View view,
            int position, long id) {
        // TODO Auto-generated method stub
        Runnable runnable = new Runnable() {

            @Override
            public void run() {
                // TODO Auto-generated method stub
                Looper.prepare();
                Context context = view.getContext();

                final String      titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
                final String        descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
                final Bitmap        bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();

                Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();
                myHandler.post(new Runnable() {

                    @Override
                    public void run() {
                        // TODO Auto-generated method stub

                        Intent intent = new Intent(getApplicationContext(), ItemDetails.class);

                        ByteArrayOutputStream stream = new ByteArrayOutputStream();
                        bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
                        byte[] bytes = stream.toByteArray(); 

                        intent.putExtra("title", titleItem);
                        intent.putExtra("BMP",bytes);
                        intent.putExtra("desc", descItem);

                        Log.i("ON ITEM CLICK", "OK");
                        startActivity(intent);
                    }
                });
                Looper.loop();                        
            }
        };
        new Thread(runnable).start();
        stopThread(Thread.currentThread());
    }
});

Can you please tell me if you see something wrong or if there is a better way of doing the same or to optimize it?

share|improve this question
    
android has background threads available (with options to callback the main thread when done) so there is no need to roll your own setup –  ratchet freak May 9 at 10:08
    
@ratchetfreak So I don't need to implement this Thread ? –  Fcps May 9 at 10:09

1 Answer 1

up vote 6 down vote accepted

@ratchetfreak's comment is correct, expanding on it here a bit:

AsyncTask is the class to use (or rather, extend and use your subclass-implementation, as AsyncTask itself is an abstract class).

You should use AsyncTask for performing operations that otherwise would block the UI-thread and thus would make your application not respond.

However, let's take a look at what your code actually does:

  • Creates a runnable
  • Gives the runnable a View, from which it takes a Context, two TextViews (and their Strings), and a Bitmap from an ImageView.
  • Then it creates and shows a toast - this should be done on the UI-thread and not in a background thread
  • Posts a runnable to your myHandler, therefore making the following code run on your UI-thread:
    • Create an intent
    • Compress a bitmap into JPEG
    • Puts some data into the intent
    • Starts a new activity
  • After the runnable has been created and a new thread started, you call a method that seems to want to stop your current thread!? Why on earth would you want to do that?? Let Android take care of the thread handling! That thread is not yours to stop!

Why do you create these threads?

The longest running operation that you perform in these threads is the Compress a bitmap into JPEG, which you run on the UI-thread anyway. The few things that you do in background is so fast that you really don't need to create a new thread for it.

If your Compress a bitmap into JPEG operation takes much time, then I would suggest you create an AsyncTask<Bitmap, Void, byte[]> to handle that. (Bitmap is the input, it does not have any progress update and therefore it says Void, and it returns a byte[] which you will send to the next activity). I don't think you need any AsyncTask at all here though.

Your new code:

With the above comments taken into consideration, here's the code you need. No extra threads. If this code causes your app to stop respond for a longer while than what is comfortable, then you can consider an AsyncTask.

myList.setOnItemClickListener(new OnItemClickListener() {

    @Override
    public void onItemClick(AdapterView<?> parent, final View view,
            int position, long id) {
        Context context = view.getContext();
        String titleItem = (String) ((TextView) view.findViewById(R.id.titleItem)).getText();
        String descItem = (String) ((TextView) view.findViewById(R.id.descItem)).getText();
        Bitmap bitmap = (Bitmap) ((BitmapDrawable) ((ImageView) view.findViewById(R.id.imgItem)).getDrawable()).getBitmap();
        Toast.makeText(context, "Item Title: " + titleItem + ", Item Desc: " + descItem, Toast.LENGTH_SHORT).show();

        Intent intent = new Intent(getApplicationContext(), ItemDetails.class);
        ByteArrayOutputStream stream = new ByteArrayOutputStream();
        bitmap.compress(Bitmap.CompressFormat.JPEG, 100, stream);
        byte[] bytes = stream.toByteArray(); 

        intent.putExtra("title", titleItem);
        intent.putExtra("BMP",bytes);
        intent.putExtra("desc", descItem);
        Log.i("ON ITEM CLICK", "OK");
        startActivity(intent);
    }
});

Normal usage of AsyncTask

Whenever you have a longer running computational-intensive operation, then you can consider an AsyncTask. Also whenever you do network operations (download a file from internet, create a long-running socket-connection, etc.), then you need an AsyncTask. As said already, I don't think your current code needs/benefits from an AsyncTask.

Consider, however...

Your ImageView known as view.findViewById(R.id.imgItem) must have gotten its drawable from somewhere. Is it loaded from a file on the external storage? Is it loaded from a resource? Rather than compressing the drawable's bitmap data and sending the byte array, consider informing your new activity about where it can find the original data. If it's a file on the external storage, pass along a String for the path it is found in. If it's a resource in your application, pass along the resource int identifier.

If the drawable is loaded from the internet or dynamically created, then I agree about compressing it as JPEG and sending the byte array to the new activity.

share|improve this answer
    
Thank you for your answer and you explanations. I'll delete my extra Thread then and it'll works. –  Fcps May 9 at 12:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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