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