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 have more than 1000 items in my GridView. When scrolling though, it appears to make the app 'shutter'. Each time I load images from Picasso GC, it increases.

import java.util.ArrayList;
import java.util.HashMap;

import android.app.Activity;
import android.content.Context;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.BaseAdapter;
import android.widget.ImageView;
import android.widget.TextView;

import com.squareup.picasso.Picasso;

public class BrandsItemAdapter extends BaseAdapter {

    public Activity activity;
    private static LayoutInflater inflater = null;

    public ArrayList<HashMap<String, String>> data;

    public HashMap<String, String> ITEM;

    public BrandsItemAdapter(Activity activity,ArrayList<HashMap<String, String>> data)
    {
        this.activity=activity;
        inflater = (LayoutInflater) activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
        this.data = data;
    }
    @Override
    public int getCount() {
        // TODO Auto-generated method stub
        return data.size();
    }

    @Override
    public Object getItem(int position) {
        // TODO Auto-generated method stub
        return position;
    }

    @Override
    public long getItemId(int position) {
        // TODO Auto-generated method stub
        return position;
    }

    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        // TODO Auto-generated method stub
        ViewHolder holder = null;
        ITEM = new HashMap<String, String>();
        ITEM = data.get(position);

        if(convertView == null)
        {
        convertView  = inflater.inflate(R.layout.department_viewadapter, parent, false);
        holder = new ViewHolder();

        holder.mTitleView = (TextView) convertView.findViewById(R.id.textView1);
        holder.imageView1 = (ImageView) convertView.findViewById(R.id.imageView1);

        convertView.setTag(holder);
        }else
        {
        holder = (ViewHolder) convertView.getTag();
        }

        try
        {
            holder.mTitleView.setText(ITEM.get("name"));
            Picasso.with(activity).load(ITEM.get("image")).placeholder(R.drawable.icon).resize(150, 150).error(R.drawable.icon).into(holder.imageView1);
            System.gc();

        }
        catch(Exception e)
        {
            e.printStackTrace();
        }
        return convertView;
    }

    private static class ViewHolder {
        TextView mTitleView;
        ImageView imageView1;
    }

}
share|improve this question

2 Answers 2

Easy things

  • auto-generated comments

    @Override
    public int getCount() {
        // TODO Auto-generated method stub
        return data.size();
    }
    

    The method is not auto-generated any more, remove the comment. (this happens in a few places).

  • brace-placement

    In Java, the opening brace { goes at the end of the line, not the start of a new line. Code like this:

        }
        catch(Exception e)
        {
            e.printStackTrace();
        }
    

    should look like:

    } catch(Exception e) {
        e.printStackTrace();
    }
    

Hard Stuff

This code is likely your problem:

holder.mTitleView.setText(ITEM.get("name"));
Picasso.with(activity).load(ITEM.get("image"))
       .placeholder(R.drawable.icon).resize(150, 150)
       .error(R.drawable.icon).into(holder.imageView1);
System.gc();

There are two things here.

Firstly, System.gc() in any code that is not directly related to the system itself, is almost always a bug. There should never be a need for this. Let the system look after itself. It really does not need your help.

The second item is that it may be taking picasso a long time to load the Images. If you are scrolling to unseen areas of the system then the code may need to be calling the getView() method a lot. You need this method to be fast.

Convert the Picasso line to be an AsyncTask, and let it populate the imageView in the background. That will make populating the views a lot faster

share|improve this answer
    
Hi rolfl, I think Picasso fetch images from background only using okhttp then why i need to change aynctask? sorry for my poor english –  prasad thangavel Mar 24 at 12:10
    
@prasadthangavel - I cannot see anything else that stands out as being a problem, and the Picasso work appears to be the heavy-duty part of the code. I do not know Picasso and how it works. The System.gc() is still a problem too. Try to confirm that Picasso is working in the background, and I recommend you also try to make it an async-task... and see if there is a difference (it's really not that hard) –  rolfl Mar 24 at 12:12
    
Maybe you need to suggest releasing an image when it scrolls out of view? –  ChrisW Mar 24 at 12:36

Firstly, what is this?

public HashMap<String, String> ITEM;
...
ITEM = new HashMap<String, String>();
ITEM = data.get(position);

This is what the getItem(int position) method is for. You're also creating a new HashMap every time a View is displayed (and immediately discarding it) which will be creating a lot of useless objects and lower performance. Your current implementation:

@Override
public Object getItem(int position) {
    // TODO Auto-generated method stub
    return position;
}

Should actually return the item at that position in your data source; for example:

@Override
public HashMap<String, String> getItem(int position) {
    return data.get(position);
}

Then in your getView() method,

ITEM = new HashMap<String, String>();
ITEM = data.get(position);

you can get rid of the public HashMap<String, String> ITEM instance, and just use:

Map<String, String> item = getItem(position);

So your getView() would start out more like:

@Override
public View getView(int position, View convertView, ViewGroup parent) {
    ViewHolder holder;
    Map<String, String> item = getItem(position);

    ...
}

You also have, at the head of your adapter:

public Activity activity;
private static LayoutInflater inflater = null;

There's no reason that the Activity instance should be public; and certainly no need for the LayoutInflater to be static. You're going to have a memory leak, since the LayoutInflater instance is tied to the Activity. NEVER store an Activity (or Fragment) in a static field. It's almost never necessary, and will almost always create a memory leak. In this case, after you finish the Activity, the adapter will retain a reference to it, and the Activity and all of its Views and their associated resources will be prevented from being garbage collected until the class is unloaded at some future time.

Another minor tip for code cleanliness is that there are a few convenience methods for getting a LayoutInflater. Either of these will work, and prevent the need to make the ugly call/cast through getSystemService():

inflater = LayoutInflater.from(activity);
inflater = activity.getLayoutInflater();

Also, you could weaken the ArrayList and HashMap declarations to just List and Map, since you don't use any methods specific to those particular implementations.

Lastly (aside from the points @rolfl has made), you have this:

private static class ViewHolder {
    TextView mTitleView;
    ImageView imageView1;
}

You should pick a naming convention and stick with it. Typical Android convention is that private member variables are prefixed with m (statics with s). Public variables (like those in this ViewHolder) shouldn't be prefixed. Suggestion:

static class ViewHolder {
    final TextView title;
    final ImageView image;

    ViewHolder(View root) {
        title = (TextView) root.findViewById(R.id.textView1);
        image = (ImageView) root.findViewById(R.id.imageView1);
    }
}

Then, you can replace:

holder = new ViewHolder();

holder.mTitleView = (TextView) convertView.findViewById(R.id.textView1);
holder.imageView1 = (ImageView) convertView.findViewById(R.id.imageView1);

convertView.setTag(holder);

with:

holder = new ViewHolder(convertView);
convertView.setTag(holder);

The final result would be something like the following; although you should certainly keep in mind @rolfl's suggestion to put the image resizing in a background thread (which is almost certainly the primary source of your stuttering problems -- although you can and should learn to profile with traceview to find the exact bottleneck):

public final class BrandsItemAdapter extends BaseAdapter {
    private final Activity mActivity;
    private final LayoutInflater mInflater;
    private final List<Map<String, String>> mData;

    public BrandsItemAdapter(Activity activity, List<Map<String, String>> data) {
        mActivity = activity;
        mInflater = activity.getLayoutInflater();
        mData = new ArrayList<Map<String, String>>(data);
    }

    @Override public int getCount() {
        return mData.size();
    }

    @Override public Map<String, String> getItem(int position) {
        return mData.get(position);
    }

    @Override public long getItemId(int position) {
        return position;
    }

    @Override public View getView(int position, View convertView, ViewGroup parent) {
        if (convertView == null) {
            convertView = inflater.inflate(R.layout.department_viewadapter, parent, false);
            convertView.setTag(new ViewHolder(convertView));
        }

        final ViewHolder holder = (ViewHolder) convertView.getTag();
        final Map<String, String> item = getItem(position);

        holder.title.setText(item.get("name"));
        Picasso.with(activity)
            .load(item.get("image"))
            .placeholder(R.drawable.icon)
            .resize(150, 150)
            .error(R.drawable.icon)
            .into(holder.image);

        return convertView;
    }

    static class ViewHolder {
        TextView title;
        ImageView image;

        ViewHolder(View root) {
            title = (TextView) root.findViewById(R.id.textView1);
            image = (ImageView) root.findViewById(R.id.imageView1);
        }
    }
}
share|improve this answer

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.