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

Question

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?

Carbuncle

Answer 1

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.

Ginny

To avoid unnecessary click listener set up got it! But can we implement this inside onCreateViewHolder() as Brucelet suggests(see below answer). – Carbuncle

@SujitYadav I suppose it would have the same effect, since onCreateViewHolder() is only called once (per ViewHolder) so whether you implement it inside your ViewHolder constructor or in onCreateViewHolder() is up to you as a personal preference. I've developed the habit of putting it in the VH, but you should do what you think is most readable and will help you understand in the future. Just avoid onBindViewHolder() for performance reasons like brucelet suggested. – Ginny

@Sujit @McAdam I tend to like doing it in onCreateViewHolder() rather than the ViewHolder constructor so that I can make my ViewHolder class static and not need to pass a reference to the adapter into the ViewHolder. But it's ultimately mostly a style choice, as there should be a one-to-one correspondence between onCreateViewHolder() and new ViewHolder(). – Athome

You don't need to pass a reference to the adapter in the viewholder? You can call getAdapterPosition() from inside the ViewHolder. See the answer I linked to. Unless I misunderstood what you meant? – Ginny

@FirstOne Thanks for the notice! I rewrote the blog a while back. I've updated the link. :) – Ginny

@Ginny if handle the click inside the VH, how do you implement selection? handle selection and selection list seem much easier to do in the adaptor. – Whereat

@Ginny I tested my recyclerView with 5 button click listener on both onBindViewHolder and viewHolder. But never seen any changes in performance, CPU uses or memory uses. Both loading properly at the same time. Is this a good approach to use button click listeners on onBindViewHolder? – Chenab

Answer 2

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;
}

Fishy

is getAdapterPosition() the best way to use , if Im sending the position and the object for a particular row to the Activity to perform CRUD operations.?bcoz when I used getLayoutPosition(), it still works! – Uranian

Great answer. By the way, pay attention that getAdapterPosition() should ONLY be placed inside the onClick() method, not outside the listner, otherwise the position will be -1. That's because when onCreateViewHolder() is called, the viewholder hasn't been attached to the view; While when the viewholder is clicked, the viewholder must have been attached to the view, thus it works. Moreover, getAdapterPosition() will be deprecated – Cloth

Answer 3

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().

Athome

Answer 4

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;
}

Edwardoedwards

Answer 5

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.

Overmuch

Answer 6

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)
}

Tuberculous

Answer 7

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.

Taite

Answer 8

You can do in this way too..

MainActivity class

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

Adapter class

Adduct