Is this a wrong way to implement a RecyclerView in Android?

Question

Im trying to implement a RecyclerView in my app. This is my Adapter class:

public class AdapterItemsList extends RecyclerView.Adapter<AdapterItemsList.ViewHolderItems>
{
    private ArrayList<CItem> items;

    public AdapterItemsList(ArrayList<CItem> items)
    {
        this.items = items;
    }

    @NonNull
    @Override
    public ViewHolderItems onCreateViewHolder(@NonNull ViewGroup parent, int viewType)
    {
        View view = LayoutInflater.from(parent.getContext())
                .inflate(R.layout.item_counter, null, false);

        return new ViewHolderItems(view);
    }

    @Override
    public void onBindViewHolder(@NonNull final ViewHolderItems holder, final int position)
    {
        holder.asignarDatos(items.get(holder.getAdapterPosition()));

        holder.itemView.setOnLongClickListener(new View.OnLongClickListener() {
            @Override
            public boolean onLongClick(View view) {

                //deleteSelectedItem() method gonna be here

                Toast.makeText(view.getContext(), String.valueOf(holder.getAdapterPosition())
                        , Toast.LENGTH_SHORT).show();
                return false;
            }
        });

        holder.counterAdd.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                Integer auxInteger = Integer.parseInt(holder.itemNumber.getText().toString());
                auxInteger += 1;
                holder.itemNumber.setText(auxInteger.toString());
                items.get(holder.getAdapterPosition()).setObjectNumber(Integer.parseInt(holder.itemNumber.getText().toString()));
            }
        });

        holder.counterSubtract.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View view) {
                Integer auxInteger = Integer.parseInt(holder.itemNumber.getText().toString());
                if (auxInteger > 0)
                {
                    auxInteger -= 1;
                    holder.itemNumber.setText(auxInteger.toString());
                    items.get(holder.getAdapterPosition()).setObjectNumber(Integer.parseInt(holder.itemNumber.getText().toString()));
                }
            }
        });
    }

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

    public class ViewHolderItems extends RecyclerView.ViewHolder
    {
        TextView itemName;
        TextView itemNumber;
        Button counterAdd;
        Button counterSubtract;

        public ViewHolderItems(View itemView)
        {
            super(itemView);

            itemName = (TextView) itemView.findViewById(R.id.textViewItemCounterName);
            itemNumber = (TextView) itemView.findViewById(R.id.textViewItemCounterNumber);
            counterAdd = (Button) itemView.findViewById(R.id.buttonItemCounterAdd);
            counterSubtract = (Button) itemView.findViewById(R.id.buttonItemCounterSubtract);
        }

        public void asignarDatos(CItem item)
        {
            itemName.setText(item.getObjectName());
            itemNumber.setText(item.getObjectNumber().toString());
        }
    }
}

The code that "creates" the adapter:

private void CreateAdapter() {
    recyclerViewItems.setAdapter(null);
    items.clear();

    //I dont post the code, but this just gets data from a SQLite database and populate "items".
    LoadClientsList();

    final AdapterItemsList adapter = new AdapterItemsList(items);

    recyclerViewItems.setAdapter(adapter);
}

As you can see, I never use "implements View.OnLongClickListener" or something like that, but I just set the OnClickListener inside onBindViewHolder. Now there is a "Toast" inside (which appears just fine when I test the app), but later on, there should be my "delete item" dialog, that gonna give the user the change to Accept or Cancel (an usual dialog). If the user press "accept" then I gonna remove the item from the list.

I gonna admit, Im not an Android or Java Ninja, so I know there are horrendous mistakes probably. But the thing is, the app is working by now (the Toast appears correctlywhen I long-press an item), but the fact that I never use "implements", while I see everyone does it in StackOverflow questions related to RecyclerViews and Adapters, make me think Im making (several) serious mistakes.

I could admit since Im not a master at Java fundamentals (some years working with COBOL already that I remember almost NULL of OOP), I don't see where my mistakes are, but still, any help will be much appreciated.

Thanks in regard!

UPDATE: Ok I was changing my code and now its like follows:

public class AdapterItemsList extends
RecyclerView.Adapter<AdapterItemsList.ViewHolderItems> implements
View.OnClickListener {
private ArrayList<CItem> items;

/* Added this */
private View.OnClickListener listener;

public AdapterItemsList(ArrayList<CItem> items)
{
    this.items = items;
}

@NonNull
@Override
public ViewHolderItems onCreateViewHolder(@NonNull ViewGroup parent, int viewType)
{
    View view = LayoutInflater.from(parent.getContext())
            .inflate(R.layout.item_counter, null, false);

  /* Added this */
    view.setOnClickListener(this);

    return new ViewHolderItems(view);
}

@Override
public void onBindViewHolder(@NonNull final ViewHolderItems holder, final int position)
{
    holder.asignarDatos(items.get(holder.getAdapterPosition()));

    holder.counterAdd.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            Integer auxInteger = Integer.parseInt(holder.itemNumber.getText().toString());
            auxInteger += 1;
            holder.itemNumber.setText(auxInteger.toString());
            items.get(holder.getAdapterPosition()).setObjectNumber(Integer.parseInt(holder.itemNumber.getText().toString()));
        }
    });

    holder.counterSubtract.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            Integer auxInteger = Integer.parseInt(holder.itemNumber.getText().toString());
            if (auxInteger > 0)
            {
                auxInteger -= 1;
                holder.itemNumber.setText(auxInteger.toString());
                items.get(holder.getAdapterPosition()).setObjectNumber(Integer.parseInt(holder.itemNumber.getText().toString()));
            }
        }
    });
}

@Override
public int getItemCount() { [...] }

/* Added this */
public void setOnClickListener(View.OnClickListener listener)
{
    this.listener = listener;
}

/* Added this */
@Override
public void onClick(View view) {
    if (listener != null)
    {
        listener.onClick(view);
    }
}

public class ViewHolderItems extends RecyclerView.ViewHolder { [...] } }

Code where I create the adapter:

private void CreateAdapter() {
    recyclerViewItems.setAdapter(null);
    items.clear();
    LoadClientsList();

    final AdapterItemsList adapter = new AdapterItemsList(items);

    /* Added this */
    adapter.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            Toast.makeText(getApplicationContext(), String.valueOf(recyclerViewItems.getChildAdapterPosition(view)), Toast.LENGTH_SHORT).show();
        }
    });

    recyclerViewItems.setAdapter(adapter);
}

The "recyclerViewItems.getChildAdapterPosition(view)" finally solves my problem (didn't mentioned before, but it was the reason why I put the "setOnClick..." inside the onBindViewHolder method, just because I needed the position of the pressed item and I couldnt figure out how on earth to access it). So now I set the listener when I create the adapter instead of when Binding the View holder, which I guess it's better. Still, I didnt change the "holder.counterAdd.setOnClickListener" and "holder.counterSubtract.setOnClickListener" lines, since I think they must be inside the onBindViewHolder method, but I guess Im still wrong on that.

Your solution is better than the `implements View.OnClickListener` because you assign each item an annonymous class instance implementing the `View.OnClickListner` interface. — Shark, Nov 18 '19 at 16:50
it's even worse because the listeners are being set in `onBindViewHolder` which will be called several times. Every time user scrolls, `onBindViewHolder` is called for every visible item and then some. So, this example is not only creating anonymous objects for fixed number of visible items, but every time `onBindViewHolder` is called — denvercoder9, Nov 18 '19 at 17:27
@sonnet that sounds atrocious now that you mention it that way. I will investigate more and see how I can use "implements". — Francisco Arbizu, Nov 18 '19 at 17:41
Would puting the "setOnClickListener" inside the "onCreateViewHolder" method change anything? At least it wont be being called many times, or am I wrong? — Francisco Arbizu, Nov 18 '19 at 18:10
and now it's probably leaking the Context of wherever this `CreateAdapter()` is being called. because anonymous classes hold a reference to their parent class (see https://stackoverflow.com/questions/5054360/do-anonymous-classes-always-maintain-a-reference-to-their-enclosing-instance) EDIT: so that's how @MikeM.- the previous one didn't leak the Context. — Shark, Nov 19 '19 at 11:34

Ben P. · Answer 1 · 2019-11-18T19:27:37.690

I disagree with most of the comments on your question, and I think that the way you've written your adapter is pretty good. It's not perfect, but there is nothing wrong with manually setting a View.OnClickListener to some (or many) views inside your ViewHolder. In fact, this is the Google-recommended way of doing things!

I would make some changes, however. The first would be to move the declaration of your click listeners into the onCreateViewHolder() method (by putting them inside the ViewHolder's constructor). The second would be to add checks for NO_POSITION to make sure you handle the case where an item has been removed from your adapter but the layout hasn't been recalculated yet.

Here is what I'd recommend:

public ViewHolderItems(View itemView)
{
    super(itemView);

    itemName = (TextView) itemView.findViewById(R.id.textViewItemCounterName);
    itemNumber = (TextView) itemView.findViewById(R.id.textViewItemCounterNumber);
    counterAdd = (Button) itemView.findViewById(R.id.buttonItemCounterAdd);
    counterSubtract = (Button) itemView.findViewById(R.id.buttonItemCounterSubtract);

    itemView.setOnLongClickListener(new View.OnLongClickListener() {
        @Override
        public boolean onLongClick(View view) {
            deleteItem();
            return true;
        }
    });

    counterAdd.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            itemAdd();
        }
    });

    counterSubtract.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View view) {
            itemSubtract();
        }
    });
}

private void deleteItem() {
    int position = getAdapterPosition();

    if (position != RecyclerView.NO_POSITION) {
        items.remove(position);
        adapter.notifyItemRemoved(position);
    }
}

private void itemAdd() {
    int position = getAdapterPosition();

    if (position != RecyclerView.NO_POSITION) {
        CItem item = items.get(position);
        item.setObjectNumber(item.getObjectNumber() + 1);
        adapter.notifyItemChanged(position);
    }
}

private void itemSubtract() {
    int position = getAdapterPosition();

    if (position != RecyclerView.NO_POSITION) {
        CItem item = items.get(position);

        if (item.getObjectNumber() > 0) {
            item.setObjectNumber(item.getObjectNumber() - 1);
            adapter.notifyItemChanged(position);
        }
    }
}

And then your onBindViewHolder() is extremely simple:

@Override
public void onBindViewHolder(@NonNull ViewHolderItems holder, int position)
{
    holder.asignarDatos(items.get(position));
}

So, let's talk about what I'm doing here.

First, I pushed all of the listener setup into the constructor of your ViewHolder. This means that the listeners are all assigned exactly once. In order to make that still function in a world where ViewHolders are recycled and attached to different items, we have to use getAdapterPosition() to make sure that we're always interacting with the correct item in your list.

We have to check for NO_POSITION because RecyclerView layout operations are asynchronous: when you delete an item from the list, there is a gap where the item doesn't exist in your adapter any more but still is displayed on screen. In this gap, the user might tap a button! In these cases, we just ignore the user input.

I also changed how you update the values of your items, and how you display those updates. Rather than getting the text and parsing it and then re-setting it, I always go through the adapter. Remember that the goal of your RecyclerView implementation is to be able to display any element in your list correctly. The best way to do this is to update the element in your data set, and then notify the RecyclerView that that element has changed, and let onBindViewHolder() take care of the required updates.

Note that you should _not_ add the `final` keyword to the `position` argument in `onBindViewHolder()`. Incremental updates (e.g. `notifyItemInserted()`) **will not** re-bind other views that have only moved around, and so you'd have a stale `position` value and that can lead to bugs. — Ben P., Nov 18 '19 at 19:33

Is this a wrong way to implement a RecyclerView in Android?

1 Answers1