Adapter for RecyclerView with supported onClick handling and select color text on last clicked item

Question

I have written an adapter for RecyclerView. It can handle onClicks for RecyclerView items and select the color name and edit TextView for the clicked item. By default the first item TextView name is the selected color. When a user clicks another item the last item TextView is named black. When the the user clicks a color, such as granadier, then the TextView is changed to that color's name. And so on.

Can my code quality and code style be improved? Additionally I'm not a fan of, when a user clicks on new item, calling notifyDataSetChanged on all items.

AudioRecordsListAdapter.kt

class AudioRecordsListAdapter :
    RecyclerView.Adapter<AudioRecordsListAdapter.AudioRecordViewHolder>() {

    var items: MutableList<AudioRecordUI> = ArrayList()
        set(value) {
            if (value.size > 0) {
                value.first().isActive = true
            }
            items.clear()
            items.addAll(value)
            notifyDataSetChanged()
        }

    var isSelectableMode = false
    var onAudioRecordClickListener: OnAudioRecordClickListener? = null

    companion object {
        var dateFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm")
        var timeFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("mm::ss")
    }

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): AudioRecordViewHolder {
        val view = LayoutInflater.from(parent.context)
            .inflate(R.layout.sound_record_item, parent, false)
        return AudioRecordViewHolder(view)
    }

    override fun getItemCount(): Int {
        return items.size
    }

    override fun onBindViewHolder(holder: AudioRecordViewHolder, position: Int) {
        holder.bind(items[position])
    }

    inner class AudioRecordViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {

        private var audioRecordUI: AudioRecordUI? = null

        fun bind(audioRecordUI: AudioRecordUI) {
            this.audioRecordUI = audioRecordUI

            val size = formatFileSize(itemView.context, audioRecordUI.getSize())

            itemView.name.text = audioRecordUI.getName()
            itemView.date.text = audioRecordUI.getDate().format(dateFormatter)
            itemView.time.text = audioRecordUI.getTime().format(timeFormatter)
            itemView.size.text = size
            itemView.select.isChecked = audioRecordUI.isSelect

            if (audioRecordUI.isActive)
                itemView.name.setTextColor(itemView.context.getColor(R.color.grenadier))
            else
                itemView.name.setTextColor(itemView.context.getColor(R.color.black))

            setClickListener(audioRecordUI)
            setLongClickListener()
        }

        private fun setClickListener(audioRecorderUI: AudioRecordUI) {
            itemView.setOnClickListener {
                if (isSelectableMode) {
                    itemView.select.isChecked = !itemView.select.isChecked
                } else {
                    onAudioRecordClickListener?.onClick(audioRecorderUI.audioRecorderEmpty)
                    clearActive()
                    audioRecorderUI.isActive = true
                    notifyDataSetChanged()
                }
            }
        }

        private fun clearActive() {
            items.forEach { it.isActive = false }
        }

        private fun setLongClickListener() {
            itemView.setOnLongClickListener {
                isSelectableMode = !isSelectableMode

                if (!isSelectableMode) {
                    clearSelect()
                }

                itemView.select.isChecked = !itemView.select.isChecked
                true
            }
        }

        private fun clearSelect() {
            items.forEach { it.isSelect = false }
            notifyDataSetChanged()
        }
    }

    interface OnAudioRecordClickListener {
        fun onClick(audioRecordEmpty: AudioRecordEmpty)
    }

}

Answer 1

I like nitwitting about kotlin, therefor only feedback about the language.

single expression functions

If your function exists out of one expression, you can write it easier.
Instead of having to write:

override fun getItemCount(): Int {
    return items.size
}

you can write:

override fun getItemCount() = items.size

code duplication

Try to remove as much duplication as possible.

if (audioRecordUI.isActive)
    itemView.name.setTextColor(itemView.context.getColor(R.color.grenadier))
else 
    itemView.name.setTextColor(itemView.context.getColor(R.color.black))

In the code above, you do three things:

1. Choose a colorId
2. Get the matching color
3. set the itemview to the color

Actions 2 and 3 are duplicated.
With a simple variable, this code becomes much clearer.

val textColorId = if(audioRecordUi.isActive) R.color.grenadier else R.color.black
itemView.name.setTextColor(itemView.context.getColor(textColorId))

scoped functions

When you need to reuse a variable multiple times after eachother, you can use a scoped function, eg also and apply.
instead of writing:

itemView.select.isChecked = !itemView.select.isChecked

you can write:

itemView.select.also { it.isChecked = !it.isChecked }

or even:

itemView.select.apply { this.isChecked = !this.isChecked }

And as you don't have to write this:

itemView.select.apply { isChecked = !isChecked }

Personal choice

I personally don't like the three lines for a simple if:

if (!isSelectableMode) {
    clearSelect()
}

If it's this short, I would place it on one line. If it would be a bit longer, two lines without brackets.

if (!isSelectableMode) clearSelect()

Answer 2

**Note: A lot of these might be very similar to tieskedh's answer but I drafted before it was here so I thought I should add it **

I'm not a fan of, when a user clicks on new item, calling notifyDataSetChanged on all items.

You can use notifyItemRangeChanged(index) or notifyItemChanged(index) in order to specify which items have changed. This is incredibly useful/important with RecyclerViews with many items as the docs say notifyDataSetChanged() is not very efficient

Other stuff

clearActive() doesn't call notifyDataSetChanged() whereas clearSelect() does which seems inconstient but this might be intended behaviour

Both dateFormatter and timeFormatter are public vals, they're never modified and I assume they're only used in this project so you can change them to private val.

var items has a custom setter which requires the whole array to be cleared and added each time. It might be worth considering adding methods to support removing and adding singular items from the RecyclerView if that fits your use case.

This is very much a question of taste and personal prefrence, where you have mutliple accesses to the same object you can use a scope function (let, apply, with etc.) to make this block of code less verbose.

itemView.name.text = audioRecordUI.getName()
            itemView.date.text = audioRecordUI.getDate().format(dateFormatter)
            itemView.time.text = audioRecordUI.getTime().format(timeFormatter)
            itemView.size.text = size
            itemView.select.isChecked = audioRecordUI.isSelect

Styling

As this is code style, this is subjective and often is something you might have agree on collabratively when working in a team.

Expression Body functions You can use expression body functions (instead of normal/block body) in Kotlin for functions that return non Unit types to remove braces. These are often easier to read/more concise especially with small functions but it's a taste thing.

override fun getItemCount() = items.size

mutableListOf() Kotlin provides the methods listOf() & mutableListOf() so you could that instead of explictly instaniating an ArrayList() like so: var items = mutableListOf<AudioRecordUI>()

Removing Explicit types

To make your code more concise, you can avoid explict time declerations like so

var dateFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm")
var timeFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("mm::ss")

It might be worth checking out Kotlin's idioms & coding-convention pages for frequently used and "more tidy" ways of wrting code compared to Java. I can't see if this is the case but you seem to be using property getters for the AudioRecordUi the way Java would. Like .getDate() audioRecordUI.getName(), in Kotlin getters are generated automatically and you should access the properties if they are exposed .name, .date etc.