hey there. got a question about the readability of...

# getting-started

[Oleg Shuliak]

hey there.
got a question about the readability of the code and how probably how to write it better. got this function:

private fun convertToEmployeeData(response: ListEmployeesResponse): List<EmployeeData> {
        val employeeDataList = mutableListOf<EmployeeData>()

        response.dataList.forEach { data ->
            val employeeData = EmployeeData(data.employeeId)

            data.attributesList.forEach { attributeValue ->
                when (attributeValue.attributeId) {
                    FIRST_NAME -> employeeData.firstName = attributeValue.stringValue?.toString()
                    LAST_NAME -> employeeData.lastName = attributeValue.stringValue?.toString()
                }
            }

            employeeDataList.add(employeeData)
        }

        return employeeDataList
    }

it works, but I’m not sure if using forEach twice is the best approach here.
Any suggestions?

[Joffrey]

The very first thing that comes to mind is extracting independent pieces of work to better reason about the code. In this case, the first example is the body of the outer forEach. It basically converts the contents of data into an instance EmployeeData - that can be a separate function (conventionally it should be an extension function on the type of data named toEmployeeData()).

After doing this, you'll realize that the whole convertToEmployeeData function is actually just a map on the `response.dataList`:

private fun convertToEmployeeData(response: ListEmployeesResponse): List<EmployeeData> =
    response.dataList.map { it.toEmployeeData() }

private fun InsertProperNameHere.toEmployeeData(): EmployeeData {
    val employeeData = EmployeeData(employeeId)

    attributesList.forEach { attributeValue ->
        when (attributeValue.attributeId) {
            FIRST_NAME -> employeeData.firstName = attributeValue.stringValue?.toString()
            LAST_NAME -> employeeData.lastName = attributeValue.stringValue?.toString()
        }
    }
    return employeeData
}

Another thing is, if EmployeeData doesn't really require mutable `firstName`/`lastName` properties, it would probably be better to make them constructor arguments instead (but that depends on whether you control this class).

[Joffrey]

A more debatable point would be the attributes parsing. If there are not many attributes, it might be clearer to just parse it as a Map, and then access that map, rather than mixing the traversal and the update of the EmployeeData instance.

As I said, probably not everyone will agree on this, though.

[Oleg Shuliak]

thanks a lot!
that’s already pretty solid advice!

[Oleg Shuliak]

unfortunately, data is the external source object (in our case grpc generated class) so extension function is not that easy to implement.

[Joffrey]

I just added the extension function to my initial response. What would be the problem here?

[Joffrey]

By extension function, I meant Kotlin's mechanism to add method-like functions to existing types without modifying stuff in the types themselves, so it should be exactly what you need.

See the doc here: https://kotlinlang.org/docs/extensions.html

[Roukanken]

(also whole convertToEmployeeData would be better defined as ListEmployeesResponse.toEmployeeDataList(): List<EmployeeData> by Kotlin conventions, or similar)

[Oleg Shuliak]

it would probably be better to make them constructor arguments instead

@Joffrey could you elaborate more on how to do that?

[Roukanken]

he means it would be better to have EmployeeData(id, firstName, lastName) constructor, instead of mutating it, if you don't need to mutate it elsewhere

[Joffrey]

I meant instead of mutating the properties after construction (employeeData.firstName = ...) you could just pass the values to the constructor: EmployeeData(employeeId, firstName, lastName).

This means those properties may not even need to be nullable nor mutable.

How is this class currently declared? Is it also generated?

[Roukanken]

Both mutability & nullability should be generally avoided, unless you need it
And if you can change the class & the only reason is for those is so you can construct the class like this, then it would be great to change it

[Oleg Shuliak]

yes, got it!
thanks a lot!

[Oleg Shuliak]

so this is what I’ve ended up with:

private fun ListEmployeesResponse.toEmployeeDataList(): List<EmployeeData> {
        return dataList.map { it.toEmployeeData() }
    }
    
    private fun EmployeeServiceOuterClass.EmployeeData.toEmployeeData() = EmployeeData(
        employeeId = employeeId,
        firstName = attributesList.firstOrNull { it.attributeId == FIRST_NAME }?.stringValue?.toString(),
        lastName = attributesList.firstOrNull { it.attributeId == LAST_NAME }?.stringValue?.toString()
    )

[Joffrey]

Yep that's a lot better already (at least IMO). The last piece I would change is the attributes access. I would either extract this one-liner into some function like private fun Attributes.getString(attributeName: String): String? or parse the list once as a Map and then access values from that map. The Map approach might not be very suitable if attributes are of heterogeneous types.

[Joffrey]

Also I'm confused about stringValue?.toString() - doesn't stringValue already return a value of String type?