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

Solution 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.

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

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

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