4
\$\begingroup\$

I'm have recyclerView which is needed for displayed for show list of my audio records. I added the ability to sort my audio records. Please take a look at the code that I use for sorting and tell me how it can be improved.

enum class AudioRecordSort {

    SortByDate {
        override fun sort(empties: List<AudioRecordEmpty>) {
            Collections.sort(empties, AudioRecordDateComparator())
            if (!isIncrease)
                empties.reversed()
        }
    },
    SortByName {
        override fun sort(empties: List<AudioRecordEmpty>) {
            Collections.sort(empties, AudioRecordNameComparator())
            if (!isIncrease)
                empties.reversed()
        }
    },
    SortBySize {
        override fun sort(empties: List<AudioRecordEmpty>) {
            Collections.sort(empties, AudioRecordSizeComparator())
            if (!isIncrease)
                empties.reversed()
        }
    },
    SortByTime {
        override fun sort(empties: List<AudioRecordEmpty>) {
            Collections.sort(empties, AudioRecordTimeComparator())
            if (!isIncrease)
                empties.reversed()
        }
    };

    val isIncrease: Boolean = true

    abstract fun sort(empties: List<AudioRecordEmpty>)
}

I would also like to say that I do not like that in each method I have to call

 if (! IsIncrease)
    empties.reversed (). 

Could this be done in a separate place for everyone?

\$\endgroup\$
5
\$\begingroup\$

You're right that the duplicate code in the sorters looks bad.

To solve this, each sorter should only define the compare function, and all the rest should be done by the enum class. Or not even this. It took me a little while of experimenting until the code compiled, but here it is:

enum class AudioRecordSort(
        private val cmp: Comparator<AudioRecordEmpty>
) {
    SortByDate(AudioRecordDateComparator()),
    SortByName(AudioRecordNameComparator()),
    SortBySize(AudioRecordSizeComparator()),
    SortByTime(AudioRecordTimeComparator());

    fun sort(empties: MutableList<AudioRecordEmpty>, asc: Boolean) {
        val cmp = if (asc) cmp else cmp.reversed()
        empties.sortWith(cmp)
    }
}

I don't think it can get any simpler than this.

I also fixed the type of the empties parameter. You had List<>, which in Kotlin is an immutable list. I changed that to MutableList<>.

I also fixed the reversed call to be reverse. I bet your IDE has warned you about the "unused result of reversed". This in turn means that you didn't test your code thoroughly before posting it here, since otherwise you would have noticed that it doesn't work for isIncrease == false.

Next time please post complete, compilable code. The missing parts for this question are:

class AudioRecordEmpty

class AudioRecordDateComparator : Comparator<AudioRecordEmpty> {
    override fun compare(o1: AudioRecordEmpty?, o2: AudioRecordEmpty?) = 0
}

class AudioRecordNameComparator : Comparator<AudioRecordEmpty> {
    override fun compare(o1: AudioRecordEmpty?, o2: AudioRecordEmpty?) = 0
}

class AudioRecordSizeComparator : Comparator<AudioRecordEmpty> {
    override fun compare(o1: AudioRecordEmpty?, o2: AudioRecordEmpty?) = 0
}

class AudioRecordTimeComparator : Comparator<AudioRecordEmpty> {
    override fun compare(o1: AudioRecordEmpty?, o2: AudioRecordEmpty?) = 0
}
\$\endgroup\$
2
  • \$\begingroup\$ How about reversing the Comparator instead of the list? \$\endgroup\$
    – D. Jurcau
    Jun 14 '20 at 16:19
  • \$\begingroup\$ Thanks for the reminder, I updated the code. I had planned to do that anyway. Do you know whether this is just a stylistic issue or if there is any measurable benefit? If any, I'd expect it to be quite small. \$\endgroup\$ Jun 14 '20 at 16:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.