Oleg Shuliak
08/24/2022, 2:51 PMprivate 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
08/24/2022, 2:56 PMforEach
. 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).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
08/24/2022, 3:01 PMdata
is the external source object (in our case grpc generated class) so extension function is not that easy to implement.Joffrey
08/24/2022, 3:05 PMRoukanken
08/24/2022, 3:07 PMconvertToEmployeeData
would be better defined as ListEmployeesResponse.toEmployeeDataList(): List<EmployeeData>
by Kotlin conventions, or similar)Oleg Shuliak
08/24/2022, 3:22 PMit would probably be better to make them constructor arguments instead@Joffrey could you elaborate more on how to do that?
Roukanken
08/24/2022, 3:26 PMEmployeeData(id, firstName, lastName)
constructor, instead of mutating it, if you don't need to mutate it elsewhereJoffrey
08/24/2022, 3:27 PMemployeeData.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
08/24/2022, 3:29 PMOleg Shuliak
08/24/2022, 3:31 PMprivate 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
08/24/2022, 3:40 PMprivate 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.stringValue?.toString()
- doesn't stringValue
already return a value of String
type?