Hey guys, Can we improve this piece of code ? ```...

# codereview

[Vivek Modi]

Hey guys, Can we improve this piece of 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
}

[Joffrey]

forEachIndexed { _, x -> ... } is pointless because it discards the index. You may as well use forEach in the first place.

[Joffrey]

Then, the pattern paragraphs = mutableListOf() + forEach { paragraphs.add(something) } can be changed using a simple map {} operator:

val paragraphs = consultationCategory.consultationTopics.orEmpty().map {
    Paragraph(type = LIST_ITEM.type, text = it.title)
}

(I added .orEmpty() to deal with the null case)

[Joffrey]

You can do the same thing one level up with val list = mutableListOf + forEach { list.add(..) }

[ephemient]

no need for the mutable list either, just

getConsultationItem()?.consultationCategories?.map { category ->
    val paragraphs = category.consultationTopics?.map {
        Paragraph(...)
    }.orEmpty()
    ContentWithTitle(...)
}.orEmpty()

[Vivek Modi]

@Joffrey nice code. Thanks for helping me.

[Vivek Modi]

@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

[Joffrey]

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

[Joffrey]

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.

[Vivek Modi]

okkk now, I got it thank you so much guys.

[Joffrey]

Note that the final code might look a bit complex. To make it look better you can extract extension functions. For instance, instead of:

val paragraphs = consultationCategory.consultationTopics.orEmpty().map {
    Paragraph(type = LIST_ITEM.type, text = it.title)
}

You could have:

val paragraphs = consultationCategory.consultationTopics.orEmpty().map { it.toParagraph() }

If you define an extension function:

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

[Vivek Modi]

Great stuff. You are amazing. Thanks it look better in my code now.

[ephemient]

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

[Joffrey]

The end game could look like this:

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)

[Vivek Modi]

@ephemient I'll check that resources. thanks

[Vivek Modi]

@Joffrey nice, I'll take a look. Many thanks

[Joffrey]

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

[Vivek Modi]

I think I don't need .orEmpty()

[Joffrey]

In your initial code you had safe-call operators (?.) so I assumed your lists were nullable, my bad

[Vivek Modi]

no problem.