Why is adding an OnClickListener inside onBindViewHolder of a RecyclerView.Adapter considered bad practice?

I have the following code for a RecyclerView.Adapter class and it works fine:

public class MyAdapter extends RecyclerView.Adapter<MyAdapter.Viewholder> {

    private List<Information> items;
    private int itemLayout;

    public MyAdapter(List<Information> items, int itemLayout){
        this.items = items;
        this.itemLayout = itemLayout;
    }

    @Override
    public Viewholder onCreateViewHolder(ViewGroup parent, int viewType) {
        View v = LayoutInflater.from(parent.getContext()).inflate(itemLayout, parent, false);
        return new Viewholder(v);
    }

    @Override
    public void onBindViewHolder(Viewholder holder, final int position) {
        Information item = items.get(position);
        holder.textView1.setText(item.Title);
        holder.textView2.setText(item.Date);

        holder.itemView.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                Toast.makeText(view.getContext(), "Recycle Click" + position, Toast.LENGTH_SHORT).show();
            }
        });

       holder.itemView.setOnLongClickListener(new View.OnLongClickListener() {
       @Override
       public boolean onLongClick(View v) {
          Toast.makeText(v.getContext(), "Recycle Click" + position, Toast.LENGTH_SHORT).show();
           return true;
       }
});
    }

    @Override
    public int getItemCount() {
        return items.size();
    }

    public class Viewholder extends RecyclerView.ViewHolder {
        public  TextView textView1;
        public TextView textView2;

        public Viewholder(View itemView) {
            super(itemView);
            textView1=(TextView) itemView.findViewById(R.id.text1);
            textView2 = (TextView) itemView.findViewById(R.id.date_row);

        }
    }
}

However, I believe it is bad practice to implement the OnClickListener in the onBindViewHolder method. Why is this bad practice, and what is a better alternative?

The reason it is better to handle your click logic inside the ViewHolder is because it allows for more explicit click listeners. As expressed in the Commonsware book:

> Clickable widgets, like a RatingBar, in a ListView row had long been in conflict with click events on rows themselves. Getting rows that can be clicked, with row contents that can also be clicked, gets a bit tricky at times. With RecyclerView, you are in more explicit control over how this sort of thing gets handled… because you are the one setting up all of the on-click handling logic.

By using the ViewHolder model you can gain a lot of benefits for click handling in a RecyclerView than previously in the ListView. I wrote about this in a blog post comparing the differences - https://androidessence.com/recyclerview-vs-listview

As for why it is better in the ViewHolder instead of in onBindViewHolder(), that is because onBindViewHolder() is called for each and every item and setting the click listener is an unnecessary option to repeat when you can call it once in your ViewHolder constructor. Then, if your click responds depends on the position of the item clicked, you can simply call getAdapterPosition() from within the ViewHolder. Here is another answer I've given that demonstrates how you can use the OnClickListener from within your ViewHolder class.

The method onBindViewHolder is called every time when you bind your view with object which just has not been seen. And every time you will add a new listener.

Instead what you should do is, attaching click listener on onCreateViewHolder

example :

@Override
public Viewholder onCreateViewHolder(ViewGroup parent, int viewType) {
     View v = LayoutInflater.from(parent.getContext()).inflate(itemLayout, parent, false);
     final ViewHolder holder = new ViewHolder(v);

     holder.itemView.setOnClickListener(new View.OnClickListener() {
         @Override
         public void onClick(View v) {
             Log.d(TAG, "position = " + holder.getAdapterPosition());
         }
     });
     return holder;
}

The onCreateViewHolder() method will be called the first several times a ViewHolder is needed of each viewType. The onBindViewHolder() method will be called every time a new item scrolls into view, or has its data change. You want to avoid any expensive operations in onBindViewHolder() because it can slow down your scrolling. This is less of a concern in onCreateViewHolder(). Thus it's generally better to create things like OnClickListeners in onCreateViewHolder() so that they only happen once per ViewHolder object. You can call getLayoutPosition() inside the listener in order to get the current position, rather than taking the position argument provided to onBindViewHolder().

Pavel provided great code example except one line in the end. You should return created holder. Not the new Viewholder(v).

@Override
public Viewholder onCreateViewHolder(ViewGroup parent, int viewType) {
     View v = LayoutInflater.from(parent.getContext()).inflate(itemLayout, parent, false);
     final ViewHolder holder = new ViewHolder(v);

     holder.itemView.setOnClickListener(new View.OnClickListener() {
         @Override
         public void onClick(View v) {
             Log.d(TAG, "position = " + holder.getAdapterPosition());
         }
     });
     return holder;
}

Per https://developer.android.com/topic/performance/vitals/render, onBindViewHolder should do its work in "much less than one millisecond" to prevent slow rendering.

> RecyclerView: Bind taking too long > > Bind (that is, onBindViewHolder(VH, int)) should be very simple, and > take much less than one millisecond for all but the most complex > items. It simply should take POJO items from your adapter's internal > item data, and call setters on views in the ViewHolder. If RV > OnBindView is taking a long time, verify that you're doing minimal > work in your bind code.

This is how I implement the clicks of my buttons in my ViewHolder instead of my onBindViewHolder. This example shows how to bind more than one button with an interface, which will not generate more objects while populating rows.

The example is in Spanish and in Kotlin, but I'm sure the logic is understandable.

/**
 * Created by Gastón Saillén on 26 December 2019
 */
class DondeComprarRecyclerAdapter(val context:Context,itemListener:RecyclerViewClickListener):RecyclerView.Adapter<BaseViewHolder<*>>() {

    interface RecyclerViewClickListener {
        fun comoLlegarOnClick(v: View?, position: Int)
        fun whatsappOnClick(v:View?,position: Int)
    }

    companion object{
        var itemClickListener: RecyclerViewClickListener? = null
    }

    init {
        itemClickListener = itemListener
    }

    private var adapterDataList = mutableListOf<Institucion>()

   fun setData(institucionesList:MutableList<Institucion>){
        this.adapterDataList = institucionesList
    }

    fun getItemAt(position:Int):Institucion = adapterDataList[position]

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): BaseViewHolder<*> {
        val view = LayoutInflater.from(context)
            .inflate(R.layout.dondecomprar_row, parent, false)
        return PuntosDeVentaViewHolder(view)
    }

    override fun getItemCount(): Int {
        return if(adapterDataList.size > 0) adapterDataList.size else 0
    }

    override fun onBindViewHolder(holder: BaseViewHolder<*>, position: Int) {
        val element = adapterDataList[position]
        when(holder){
            is PuntosDeVentaViewHolder -> holder.bind(element)
            else -> throw IllegalArgumentException()
        }

    }

    inner class PuntosDeVentaViewHolder(itemView: View):BaseViewHolder<Institucion>(itemView),View.OnClickListener{

        override fun bind(item: Institucion) {
            itemView.txtTitleDondeComprar.text = item.titulo
            itemView.txtDireccionDondeComprar.text = item.direccion
            itemView.txtHorarioAtencDondeComprar.text = item.horario
            itemView.btnComoLlegar.setOnClickListener(this)
            itemView.btnWhatsapp.setOnClickListener(this)
        }

        override fun onClick(v: View?) {
            when(v!!.id){
                R.id.btnComoLlegar -> {
                    itemClickListener?.comoLlegarOnClick(v, adapterPosition)
                }

                R.id.btnWhatsapp -> {
                    itemClickListener?.whatsappOnClick(v,adapterPosition)
                }
            }
        }
    }
}

And the BaseViewHolder to implement in each adapter

/**
 * Created by Gastón Saillén on 27 December 2019
 */
abstract class BaseViewHolder<T>(itemView: View) : RecyclerView.ViewHolder(itemView) {
    abstract fun bind(item: T)
}

i faced a small problem which i want to share in the answers if someone else also face it. i had image and text to show in Recycleview as Cardview. Thus my code according to recommendations should be as follow.

@Override
    public MyViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
        View itemView = LayoutInflater.from(parent.getContext())
                .inflate(R.layout.books_item_row, parent, false);

          final MyViewHolder holder = new MyViewHolder(itemView);
        holder.itemView.setOnClickListener(new View.OnClickListener() {
            @Override
      public void onClick(View v) {
  Toast.makeText(getActivity(), "Recycle Click", Toast.LENGTH_LONG).show();
            }
        });
         return holder;
    }

however when i will click the card in recycle view it will not work as the itemview is below the image. Thus i slightly changed the code as follow.

 @Override
        public MyViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
            View itemView = LayoutInflater.from(parent.getContext())
                    .inflate(R.layout.books_item_row, parent, false);

              final MyViewHolder holder = new MyViewHolder(itemView);
            holder.thumbnail.setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View v) {
                    //Log.d(TAG, "position = " + holder.getAdapterPosition());
                        Toast.makeText(getActivity(), "Recycle Click", Toast.LENGTH_LONG).show();
                    }
            });
                 return holder;
        }

i.e instead of itemview now person will have to click on thumbnail or image.

You can do in this way too..

MainActivity class

in this multiple type of interface triggers you can achieve this...

Adapter class