Merge objects in Kotlin if they are updated

Question

I am very new to Kotlin and I am trying to achieve this:

• I get a batch of PersonDetails to save or update
• In the batch, the same PersonDetails with name is different locale can be present.
• The end goal of this method is to merge the batch itself and also merge the existing entries in Database then return the updated data structure for saving into DB.

I am not very happy with the way I have coded, can you please review this code and give your review comments.

fun doProcess(batchToProcess: List<PersonDetail>) : Map<Long, PersonDetail> {
 val idsInBatch = batchToProcess.map { it.lcmId }.toList()
 val personDetailsFromDb = personDetailsRepository.getBatch(idsInBatch)
 .map { it.lcmId to it }
 .toMap()
 val batchesOfPersonDetails = mutableMapOf<Long, PersonDetail>()
 batchToProcess.map { aPerson ->
 var personDetail = batchesOfPersonDetails[aPerson.lcmId]
 if (personDetail == null) {
 personDetail = personDetailsFromDb[aPerson.lcmId]
 }
 val updatedPersonDetail = when {
 personDetail == null -> aPerson.toPersonDetail()
 personDetail.isActive != aPerson.isActive -> {
 personDetail.isActive = personDetail.isActive
 personDetail
 }
 personDetail.localeDetails[aPerson.localeCode] != aPerson.localeName -> {
 personDetail.localeDetails[aPerson.localeCode] = aPerson.localeName
 personDetail
 }
 else -> {
 logger.info { "Duplicate_Event for id ${aPerson.lcmId}" }
 personDetail
 }
 }
 batchesOfPersonDetails[updatedPersonDetail.lcmId] = updatedPersonDetail
 }
 return batchesOfPersonDetails
}

• tieskedh: Did you mean to do only one update inside when at a time or all the updates at the same time?

Answer 1

To make your code understandable, you should extract the code for merging a single PersonDetail into a separate function:

fun merge(fresh: PersonDetail, fromDb: PersonDetail): PersonDetail {
}

This function is where the main complexity happens, and it should be easy to write unit tests for this part of the code. And you should definitely write unit tests, after all you're dealing with details of actual people here, so you better get it correct from the beginning.

In this merge function you can have much shorter variable names. Since you are dealing with persons only, you can remove the word person from the variable names, like I already did in the function declaration above. You can remove the word detail as well.

• NewUser: Thanks, removing person and detail makes sense.

Answer 2

It's hard to follow exactly your logic. Pretty sure this line is wrong:

personDetail.isActive = personDetail.isActive

You probably meant to assign to something else as you are assigning same value (maybe personDetail.isActive = aPerson.isActive)

Another fishy thing is, that you have those 2 conditions in "when" for assigning 'active' and 'locale' stuff. Maybe it's correct, but maybe both can happen at same time. In that case when is bad as it executes only first matching.

Overall I would rewrite mapping function to form, that makes more sense to me. I think you can first setup return instance in one place rather than in different places and then at once modify it however you want. One question is logging and if it is necessary. Without logging it is even simpler, but otherwise I'd go with 1 if with or conditions and put logging in else or use else if + else in case, there is no or condition. Remove comments to have same functionality as in when.

batchToProcess.map { aPerson ->
 val personDetail = batchesOfPersonDetails.getOrDefault(
 aPerson.lcmId,
 personDetailsFromDb.getOrDefault(aPerson.lcmId, aPerson.toPersonDetail())
 )
 if (personDetail.isActive != aPerson.isActive) { //or both conditions with OR and then all changed content in one branch
 personDetail.isActive = aPerson.isActive // correct
 } /* else */
 if (personDetail.localeDetails[aPerson.localeCode] != aPerson.localeName) {
 personDetail.localeDetails[aPerson.localeCode] = aPerson.localeName
 } /* else { do logging } */
 batchesOfPersonDetails[personDetail.lcmId] = personDetail
}