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?

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).

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

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

(also whole

convertToEmployeeData

would be better defined as

ListEmployeesResponse.toEmployeeDataList(): List<EmployeeData>

by Kotlin conventions, or similar)

it would probably be better to make them constructor arguments instead

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

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

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?

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

yes, got it! thanks a lot!

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.