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?

score 73 · Accepted Answer · edited Jun 10 '19 at 07:51

73

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.

edited Jun 10 '19 at 07:51

bryansoftdev

69
10

answered Nov 21 '15 at 16:47

AdamMc331

16,492
10
71
133

2

To avoid unnecessary click listener set up got it! But can we implement this inside onCreateViewHolder() as Brucelet suggests(see below answer). – Sujit Yadav Nov 21 '15 at 17:52
1

@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. – AdamMc331 Nov 21 '15 at 18:01
@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()`. – RussHWolf Nov 21 '15 at 18:19
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? – AdamMc331 Nov 21 '15 at 18:32
@FirstOne Thanks for the notice! I rewrote the blog a while back. I've updated the link. :) – AdamMc331 Aug 29 '18 at 19:27
@AdamMc331 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. – CodingTT May 16 '19 at 22:34
@AdamMc331 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? – M DEV Oct 28 '22 at 14:45

score 19 · Answer 2 · edited Jul 22 '19 at 13:22

19

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

edited Jul 22 '19 at 13:22

user158

12,852
7
62
94

answered Feb 05 '16 at 17:06

Pavel Kozemirov

219
2
7

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! – adi Dec 22 '19 at 11:14
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](https://developer.android.com/reference/androidx/recyclerview/widget/RecyclerView.ViewHolder#getAdapterPosition()) – vainquit Mar 23 '21 at 16:57

score 16 · Answer 3 · answered Nov 21 '15 at 16:43

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

score 7 · Answer 4 · answered Mar 29 '18 at 16:29

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

score 4 · Answer 5 · edited Aug 24 '19 at 17:58

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.

score 0 · Answer 6 · edited Mar 12 '20 at 21:49

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

score 0 · Answer 7 · answered Apr 01 '20 at 14:44

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.

score 0 · Answer 8 · answered Oct 26 '20 at 10:21

0

You can do in this way too..

MainActivity class

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

Adapter class

answered Oct 26 '20 at 10:21

P Sekhar

176
1
7

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

8 Answers8

Linked