Create two teams from a list of players with minimum of one goalkeeper per team

Question

I wanted to create a simple method that creates two random teams for football/soccer match from a list of players with a requirement that the goalkeepers should be in different teams (so that one team should have at least one goalkeeper). It's a simple task and I wanted this method to be as clean as possible, let me know what do you think. cheers!

fun shuffleTeams() {
    val availablePlayers = playersList.toMutableList()
    val teamA = mutableListOf<Player>()
    val teamB = mutableListOf<Player>()

    val goalkeepers = availablePlayers.groupBy { player -> player.isGoalkeeper }.values.first { playersLists ->
        playersLists.all { player -> player.isGoalkeeper }
    }
    if (goalkeepers.size >= 2) { availablePlayers.removeAll(goalkeepers.take(2)) }

    availablePlayers.shuffle()
    val teams = availablePlayers.chunked(if (availablePlayers.size % 2 == 0) (availablePlayers.size / 2) else (availablePlayers.size / 2) + 1)

    if (goalkeepers.size >= 2) {
        teamA.add(goalkeepers[0])
        teamB.add(goalkeepers[1])
    }

    teamA.addAll(teams[0])
    teamB.addAll(teams[1])

    teamA.forEach { println("teamA - ${it.name}, ${it.isGoalkeeper}") }
    teamB.forEach { println("teamB - ${it.name}, ${it.isGoalkeeper}") }
}
private val playersList = listOf(Player("A", true), Player("B"), Player("C"), Player("D"), Player("E"), Player("F"),
    Player("G"), Player("H", true), Player("I", true), Player("J"), Player("K"), Player("L"), Player("M"))

data class Player(
    val name: String,
    val isGoalkeeper: Boolean = false
)

Answer 1

{ player -> player.isGoalkeeper } can be replaced with (Player::isGoalkeeper) in all the places you're using it. This is recommended syntax from Jetbrains because it's self-documenting...no extra variable name introduced so it's simpler to read and know what the types are.

Your one-line if-statement I think most programmers would avoid. It's more readable to break out the conditional contents into their own line(s).

Your way of getting a list of all goalkeepers is quite convoluted. You could simply use filter(), or partition() if you needed the list of non-goalkeeers (which you aren't using).

Otherwise, I don't see any other cleanliness issues.

I think this algorithm could be simpler though. I think it tends to be cleaner not to have to add and subtract from collections multiple times. I find your code a little bit hard to follow.

fun shuffleTeams() {
    val goalkeepers = playersList.filter(Player::isGoalkeeper).shuffled().take(2)
    require(goalkeepers.size >= 2) { "Not enough goal keepers!" }
    val others = (playersList - goalkeepers).shuffled()
    val teamA = others.take(others.size / 2) + goalkeepers[0]
    val teamB = others.drop(others.size / 2) + goalkeepers[1]

    teamA.forEach { println("teamA - ${it.name}, ${it.isGoalkeeper}") }
    teamB.forEach { println("teamB - ${it.name}, ${it.isGoalkeeper}") }
}

However, I think it would usually be desirable evenly divide people who can goalkeep between teams so players can sub out. To do it that way I would use:

fun shuffleTeams() {
    val (goalkeepers, others) = playersList.shuffled().partition(Player::isGoalkeeper)
    require(goalkeepers.size >= 2) { "Not enough goal keepers!" }
    val teamA = goalkeepers.take(goalkeepers.size / 2) + others.drop(others.size / 2)
    val teamB = goalkeepers.drop(goalkeepers.size / 2) + others.take(others.size / 2)

    teamA.forEach { println("teamA - ${it.name}, ${it.isGoalkeeper}") }
    teamB.forEach { println("teamB - ${it.name}, ${it.isGoalkeeper}") }
}

Note that I stagger the use of take and drop between the two teams. This is so if there is both an odd number of goalkeepers and an odd number of non-goalkeepers, neither team will receive both rounding errors in their favor and end up with two more players than the other.