Round Robin Tournament

Question

RoundRobin is an implementation of the Round-robin style tournament:

public class RoundRobin implements Tournament {
    private final Game game;
    private final List<Player> allPlayers;
    private final List<Player> remainingPlayers;

    public RoundRobin(Game game){
        this.game = game;
        this.allPlayers = new ArrayList<>();
        this.remainingPlayers = new ArrayList<>();
    }

    /**
     * Starts a round of a game, selecting players in a round-robin fashion
     */
    @Override
    public Round nextRound() {
        int playersRemaining = getPlayerCount();
        List<Player> nextPlayers = new ArrayList<>();
        while (playersRemaining > 0) {
            playersRemaining = addPlayers(nextPlayers, playersRemaining);
            if (remainingPlayers.isEmpty()) {
                populateRemainingPlayers(nextPlayers);
            }
        }

        return new Round(game, nextPlayers);
    }

    /**
     * Repopulates the remaining player list, avoiding players that have already been used if the game disallows it
     */
    private void populateRemainingPlayers(List<Player> usedPlayers){
        if (game.allowDuplicatePlayers()) {
            remainingPlayers.addAll(allPlayers);
        } else {
            Set<Player> addedPlayers = new HashSet<>(usedPlayers);
            remainingPlayers.addAll(allPlayers.stream()
                    .filter(addedPlayers::contains).collect(Collectors.toList()));
        }
        Collections.shuffle(remainingPlayers, Controller.random);
    }

    /**
     * Adds players to the upcoming game
     */
    private int addPlayers(List<Player> nextPlayers, int playersRemaining) {
        if (remainingPlayers.size() < playersRemaining) {
            nextPlayers.addAll(remainingPlayers);
            playersRemaining-=remainingPlayers.size();
            remainingPlayers.clear();
            return playersRemaining;
        }
        Set<Player> chosenPlayers = new HashSet<>(remainingPlayers.subList(0, playersRemaining));
        nextPlayers.addAll(chosenPlayers);
        remainingPlayers.removeAll(chosenPlayers);
        return 0;
    }

    /**
     * Checks to make sure there are enough players to play, and returns the number of players we are going to play with
     */
    private int getPlayerCount(){
        if (allPlayers.size() < Math.max(game.minPlayers(), 1) && !game.allowDuplicatePlayers()){
            throw new NotEnoughPlayersException();
        }
        int playerCount = Math.min(allPlayers.size(), game.maxPlayers());
        if (playerCount == -1){
            return allPlayers.size();
        }
        return playerCount;
    }

    /**
     * Adds players to the pool
     */
    @Override
    public void addPlayers(List<Player> players) {
        this.allPlayers.addAll(players);
        this.remainingPlayers.addAll(players);
        Collections.shuffle(this.remainingPlayers, Controller.random);
    }
}

Some notes:

• This is for a personal project, so feel free to overhaul the entire thing if you'd like
• Game stores data/logic of the actual game, while a Round is an instance of a game.
• Games can define a minimum/maximum number of players. If the maximum is -1, then it allows for infinite amount of players.
• A round should have as many players as possible.
• Oftentimes games don't care if there are multiple instances of the same player in a game.

Answer 1

This is somehow the most obfuscated code that I seen on this year perhaps, but it's not that bad, at least not after spending some time to figure it out. But that's why we are here, to make it better.

For this review I will be replacing Controller.random with new Random(1) for simplification.

public Round nextRound() {
    int playersRemaining = getPlayerCount();
    List<Player> nextPlayers = new ArrayList<>();
    while (playersRemaining > 0) {
        playersRemaining = addPlayers(nextPlayers, playersRemaining);
        if (remainingPlayers.isEmpty()) {
            populateRemainingPlayers(nextPlayers);
        }
    }
    return new Round(game, nextPlayers);
}

There are some things that are not intuitive on your while body.

• addPlayers update playersRemaining, letting us being uncertain on when the while exits
• seems that populateRemainingPlayers can run more than once
• You are adding players from remainingPlayers in populateRemainingPlayers and removing on addPlayers and in the end it's hard to tell what you are really trying to achieve on your algorithm

---

public void addPlayers(List<Player> players) {
    this.allPlayers.addAll(players);
    this.remainingPlayers.addAll(players);
    Collections.shuffle(this.remainingPlayers, new Random(1));    
}

If the game had a list of all players you probably could initialize the lists on the constructor instead of doing it on this method.

---

I tend to hate method that receive a list as a parameter and then add elements to it (like addPlayers(List<Player> nextPlayers, int playersRemaining)), but I have some good reasons to do so:

• The caller needs to instantiate the list
• The method that receives the list may need to call other methods with that same list
• Which means that elements can be added to the list in different methods
• You should separate data handling from logic handling
• You can test more easily a method that returns a list that is ready to use

---

Let's resume your need so we can achieve a better solution. It may be a little bit bad worded but I did the best I could.

• If the game does not allowDuplicatePlayers then the player list should have enough players to play the game, however it may have more
• If the list have more players than necessary, then we want to keep track of the last player that was able "to play", so all the players are able to play
• After all the players have played it seems you want to have a different player order
• If the game allowDuplicatePlayers you can take as many players needed from the player list sequentially, going back to start if necessary taking the same player more than once

Let's try to implement something along those lines:

private int idxLastPlayer;
public Round nextRound() {
    return new Round(game, getNextPlayers());
}

private List<Player> getNextPlayers(){
    int playersRemaining = getPlayerCount();
    List<Player> players = new ArrayList<Player>();
    for(int i = 0; i < playersRemaining; ++i){
        //with idxLastPlayer we still round robin players if game.allowDuplicatePlayers()
        int idx = (i + idxLastPlayer)%remainingPlayers.size();
        players.add(remainingPlayers.get(idx));
        ++idxLastPlayer;
    }
    updateRemainingPlayers();
    return players;
}

private void updateRemainingPlayers(){
    int playersRemaining = getPlayerCount();
    if (!game.allowDuplicatePlayers()){
        int nPlayers = Math.max(playersRemaining, remainingPlayers.size() - 1);
        for(int i = 0; i < nPlayers; ++i){
            remainingPlayers.remove(0); //removing from the start it's not the best but let's not worry about it
        }
    }else{
        //we can keep remainingPlayers always on the same state
        //Todo Should we shuffle players when we reach the end? But we could leap to an index > 0...
        return;
    }
    //if we no longer have enough players we need to reset remainingPlayers with allPlayers 
    if(remainingPlayers.size() == 0 || 
        remainingPlayers.size() < playersRemaining && !game.allowDuplicatePlayers()) //this will discard the last allPlayers.size() % playersRemaining players
    {
        //Todo prioritize the players that were still on remainingPlayers?
        remainingPlayers = new ArrayList<Player>(allPlayers);
        //Start the next round on a different order
        Collections.shuffle(this.remainingPlayers, new Random(1));
    }
}