ViewHolder pattern. Common mistakes.

Probably most of you already used the known ViewHolder pattern. If not then you should. I've seen some implementations here and there, however many of them are being wrongly implemented.

Let's have a look at this implementation of a ViewHolder on a simple ArrayAdapter:

public class ExampleAdapter extends ArrayAdapter<String> {
    private List<String> mData;

    public ExampleAdapter(Context context, List<String> objects) {
        super(context, 0, objects);
        mData = objects;
    }

    @Override
    public View getView(final int position, View convertView, ViewGroup parent) {
        ViewHolder holder;
        if (convertView == null) {
            convertView = LayoutInflater.from(getContext()).inflate(R.layout.simple_list_item_1, null);
            holder = new ViewHolder();
            holder.textView = (TextView) convertView;
            convertView.setTag(holder);
        } else {
            holder = (ViewHolder) convertView.getTag();
        }
        holder.textView.setText(mData.get(position));
        holder.textView.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                Toast.makeText(getContext(), "Clicked in '" + mData.get(position) + "'", Toast.LENGTH_SHORT).show();
            }
        });

        return convertView;
    }

    private class ViewHolder {
        public TextView textView;
    }
}

Did you spot any mistake? If you didn't I don't blame you. I made some of them a couple of times too.

So let's start with the most obvious one:

LayoutInflater.from(getContext()).inflate(R.layout.simple_list_item_1, null);

Avoid passing null as the parent layout whenever you inflate a view. Otherwise your inflated view might have an incorrect layout display. Also when you pass the parent view you need to specify if you want the inflater to attach it to the parent or not. In this case that's not necessary so you should pass false:

LayoutInflater.from(getContext()).inflate(R.layout.simple_list_item_1, parent, false);

Now that we've fixed this one let's have a look on another one:

private class ViewHolder

Inner classes or non-static nested classes will keep a reference to the outer class. This means that for each ViewHolder object you create, it will contain a reference to the whole Adapter. This could lead to some serious memory problems so you should always declare your ViewHolder as static unless you create a new class file for it.

static class ViewHolder

Another common mistake we have here is to create and set listeners, like View.OnClickListener, every time we load a new view. The views are recycled so it means the listeners will get recycled too. So why not use them instead of overriding them every time we load the view?

Start by moving the listener routine inside the convertView==null condition. This will remove the constant allocation of onClick listeners. However, this wont refresh the listeners of our view. Which means that we'll end up with incorrect click events because the position is not updated. So how can we fix this? By saving the position of the adapter in the ViewHolder. Create a position integer on your ViewHolder

static class ViewHolder {
        public TextView textView;
        public int position;
}

and save it every time you reload the view.

holder.position = position;
(...)

Then, every time you need to get the position you clicked just grab the position from the ViewHolder:

holder.textView.setOnClickListener(new View.OnClickListener() {

    @Override
    public void onClick(View v) {
        Toast.makeText(getContext(), "Clicked in '" + mData.get(holder.position) + "'", Toast.LENGTH_SHORT).show();
    }
});

Now you may ask... What about RecyclerView.ViewHolder? Well... The position hack was already added so every time you need to refer to the position inside a listener just use viewHolder.getPosition(); viewHolder.getAdapterPosition().  However you might want to check if you're not setting your listeners on onBindViewHolder instead of onCreateViewHolder. Same logic applies. The bind method will get called when a view get's recycled so you don't want to set your listener on this method. Use onCreateViewHolder and ViewHolder.getPosition(); and you are ready to go.


Finally, you should always avoid allocate big objects on the bind/recycling view method.

Implementing ViewHolder is a must-have on adapters and if you're going to do it, try doing it right.

The code for the fixed adapter can be found here: https://gist.github.com/kanytu/343adf3d246f49d683c9