https://kotlinlang.org logo
#codereview
Title
# codereview
v

Vivek Modi

06/15/2022, 3:18 PM
Hey guys, Can we improve this piece of code ?
Copy code
fun getConsultationCoverageList(): List<ContentWithTitle> {
    val list = mutableListOf<ContentWithTitle>()
    val consultation = getConsultationItem()
    consultation?.consultationCategories?.forEachIndexed { _, consultationCategory ->
        val paragraphs = mutableListOf<Paragraph>()
        consultationCategory.consultationTopics?.forEachIndexed { _, consultationTopics ->
            paragraphs.add(Paragraph(type = LIST_ITEM.type, text = consultationTopics.title))
        }

        list.add(ContentWithTitle(title = consultationCategory.title, content = Paragraphs(paragraphs), description = consultationCategory.description))
    }
    return list
}
j

Joffrey

06/15/2022, 3:19 PM
forEachIndexed { _, x -> ... }
is pointless because it discards the index. You may as well use
forEach
in the first place.
Then, the pattern
paragraphs = mutableListOf()
+
forEach { paragraphs.add(something) }
can be changed using a simple
map {}
operator:
Copy code
val paragraphs = consultationCategory.consultationTopics.orEmpty().map {
    Paragraph(type = LIST_ITEM.type, text = it.title)
}
(I added
.orEmpty()
to deal with the null case)
You can do the same thing one level up with
val list = mutableListOf
+
forEach { list.add(..) }
e

ephemient

06/15/2022, 3:22 PM
no need for the mutable list either, just
Copy code
getConsultationItem()?.consultationCategories?.map { category ->
    val paragraphs = category.consultationTopics?.map {
        Paragraph(...)
    }.orEmpty()
    ContentWithTitle(...)
}.orEmpty()
v

Vivek Modi

06/15/2022, 3:27 PM
@Joffrey nice code. Thanks for helping me.
@ephemient I am not in expert in time complexity but when i am using
2 foreach
the my complexity is
n^2
. So what will be good effect when using
map
? Thanks
j

Joffrey

06/15/2022, 3:30 PM
Using
map
is just a more standard way of doing the same thing. The time complexity is unchanged. It is just an abstraction to better convey your intent
Your initial code was saying "create a mutable list, then for each element of that other list, add it to my mutable list, now return my mutable list". With
map
you're instead saying "convert this list into a new list where each item is transformed this way". The code does the same thing, but it expresses your actual intent at a higher level, which is usually faster to read and understand.
v

Vivek Modi

06/15/2022, 3:33 PM
okkk now, I got it thank you so much guys.
j

Joffrey

06/15/2022, 3:34 PM
Note that the final code might look a bit complex. To make it look better you can extract extension functions. For instance, instead of:
Copy code
val paragraphs = consultationCategory.consultationTopics.orEmpty().map {
    Paragraph(type = LIST_ITEM.type, text = it.title)
}
You could have:
Copy code
val paragraphs = consultationCategory.consultationTopics.orEmpty().map { it.toParagraph() }
If you define an extension function:
Copy code
private fun ConsultationTopic.toParagraph() = Paragraph(type = LIST_ITEM.type, text = title)
This usually reads better, again because of the higher level of abstraction, hiding the details
v

Vivek Modi

06/15/2022, 3:38 PM
Great stuff. You are amazing. Thanks it look better in my code now.
e

ephemient

06/15/2022, 3:42 PM
have you read the documentation https://kotlinlang.org/docs/collections-overview.html or worked through other resources such as https://kotlinlang.org/docs/koans.html or any other resources? you have many questions that are already covered
j

Joffrey

06/15/2022, 3:44 PM
The end game could look like this:
Copy code
fun getConsultationCoverageList(): List<ContentWithTitle> =
    getConsultationItem()?.consultationCategories.orEmpty().map { it.toContentWithTitle() }

private fun ConsultationCategory.toContentWithTitle(): ContentWithTitle =
    ContentWithTitle(title = title, content = paragraphs, description = description)

private val ConsultationCategory.paragraphs
    get() = Paragraphs(consultationTopics.orEmpty().map { it.toParagraph() })

private fun ConsultationTopic.toParagraph() = Paragraph(type = LIST_ITEM.type, text = title)
v

Vivek Modi

06/15/2022, 3:49 PM
@ephemient I'll check that resources. thanks
@Joffrey nice, I'll take a look. Many thanks
j

Joffrey

06/15/2022, 3:53 PM
Another point that we haven't covered so far is the nullability of your lists in the first place. If you don't care about the distinction between
null
or empty list for your use case (which is the case in this piece of code), you should probably make them non-nullable in the first place in your classes. This will get rid of those
.orEmpty()
everywhere. That is, unless those classes are defined in Java and you have no choice but to deal with null
v

Vivek Modi

06/15/2022, 3:54 PM
I think I don't need
.orEmpty()
j

Joffrey

06/15/2022, 3:55 PM
In your initial code you had safe-call operators (
?.
) so I assumed your lists were nullable, my bad
v

Vivek Modi

06/15/2022, 3:56 PM
no problem.
4 Views