Can any one enhance this code :rose: ```private f...

# codereview

[hisham bin awiad]

Can any one enhance this code 🌹

private fun getCustomEmployeeShifts(
  employeeShifts: List<Shifts>?,
  approvedOvertimes: List<ApprovedOvertimesModel>?
): List<Shifts> {

  val resultList = mutableListOf<Shifts>()
  var sh: Shifts

  employeeShifts?.forEach { shiftsModel ->
    sh = shiftsModel
    approvedOvertimes?.forEach { appOverModel ->
      if (sh.id == appOverModel.employeeShiftId) {
        sh.approvedOvertimes.add(appOverModel)
      }
    }
    resultList.add(sh)
  }

  return resultList.toList()
}

[Joffrey]

what do the types look like? (Shifts and ApprovedOvertimeModel)

[hisham bin awiad]

data class(…)

[Joffrey]

Well yes, but the goal was for people to be able to copy-paste and use your code to refactor it without compile errors

[Joffrey]

Here are a few changes that I would start with:

1. why are you parameters nullable? It seems the function is not very useful if any of those are null. Why not make them non-nullable and just not call the function if they are null?
2. avoid var when unnecessary. Here just declare the sh variable inside the forEach and it can be a val . In fact, once you do that, you may realize you already have that val (shiftsModel) so you don't need sh at all.
3. the resultList serves no purpose, you modify the existing list in place and add all elements from it to the result, so since both lists are read-only, you are basically returning the same list. If you really want a defensive copy of that list, you can still return a list using toList() - but do it on the original. That being said, returning another list usually implies that you didn't modify the original, so in the name of least surprise I would advise to not return anything at all, so people know you're actually mutating the parameters
Which would give the following:

data class Shifts(
    val id: String,
    val approvedOvertimes: MutableList<ApprovedOvertimesModel>,
)

data class ApprovedOvertimesModel(val employeeShiftId: String)

private fun getCustomEmployeeShifts(
    employeeShifts: List<Shifts>,
    approvedOvertimes: List<ApprovedOvertimesModel>
) {
    employeeShifts.forEach { shiftsModel ->
        approvedOvertimes.forEach { appOverModel ->
            if (shiftsModel.id == appOverModel.employeeShiftId) {
                shiftsModel.approvedOvertimes.add(appOverModel)
            }
        }
    }
}

[Javier]

can you share your data classes? I would avoid a lot of mutability things you have, or you are looking for performance?

[Javier]

Looks like you have something so

data class Shifts(val id: Int, val approvedOvertimes: MutableList<ApprovedOvertimesModel>)

data class ApprovedOvertimesModel(val employeeShiftId: Int)

fun getCustomEmployeeShifts(
    employeeShifts: List<Shifts>?,
    approvedOvertimes: List<ApprovedOvertimesModel>?
): List<Shifts> {

    val resultList = mutableListOf<Shifts>()
    var sh: Shifts

    employeeShifts?.forEach { shiftsModel ->
        sh = shiftsModel
        approvedOvertimes?.forEach { appOverModel ->
            if (sh.id == appOverModel.employeeShiftId) {
                sh.approvedOvertimes.add(appOverModel)
            }
        }
        resultList.add(sh)
    }

    return resultList.toList()
}

[Javier]

As @Joffrey before.

You can maybe use map instead of forEach. You can return a buildList { } too.

[Joffrey]

As @Javier said, I would also prefer returning a new list (with new Shifts instances) rather than mutating existing instances, unless this code path is super performance-critical. It usually avoids a lot of bugs.

[Robert Williams]

A couple of things already covered but worth elaborating on:
Nullable lists are often a bad idea (unless you have a really specific reason why null list and empty list are different things)
Mutable data classes (or including mutable containers) are usually a bad idea because they force behaviours into objects which should be just data. Would recommend using copy method to modify things

[Robert Williams]

For your code specifically it’ll likely be more readable and performant with grouping:

val overtimesByShift = approvedOvertimes?.groupBy { it.employeeShiftId }
employeeShifts?.forEach { it.approvedOvertimes = overtimesByShift[it.id] }