private Optional<Player> playerWithMostCards()
    {
        Player scoringPlayer = null;
        int maxCount = 0;

        for(Player p : players)
        {
            final int count = p.pile.size();

            if(count > maxCount)
            {
                scoringPlayer = p;
                maxCount = count;
            }
            else if(count == maxCount)
            {
                scoringPlayer = null;
            }
        }

        return Optional.of(scoringPlayer);
    }

    private Optional<Player> playerWithMostSevens()
    {
        Player scoringPlayer = null;
        int maxCount = 0;

        for(Player p : players)
        {
            int count = 0;

            for(Card c : p.pile)
            {
                if(c.is(Card.Value.SEVEN))
                {
                    count  ;
                }
            }

            if(count > maxCount)
            {
                scoringPlayer = p;
                maxCount = count;
            }
            else if(count == maxCount)
            {
                scoringPlayer = null;
            }
        }

        return Optional.of(scoringPlayer);
    }

    private Optional<Player> playerWithMostSpades()
    {
        Player scoringPlayer = null;
        int maxCount = 0;

        for(Player p : players)
        {
            int count = 0;

            for(Card c : p.pile)
            {
                if(c.is(Card.Suit.SPADES))
                {
                    count  ;
                }
            }

            if(count > maxCount)
            {
                scoringPlayer = p;
                maxCount = count;
            }
            else if(count == maxCount)
            {
                scoringPlayer = null;
            }
        }

        return Optional.of(scoringPlayer);
    }
    
    private Optional<Player> playerWithSevenOfSpades()
    {
        for(Player p : players)
        {
            for(Card c : p.pile)
            {
                if(c == new Card("7S"))
                {
                    return Optional.of(p);
                }
            }
        }

        return Optional.empty();
    }

    private void updateScores()
    {
        for(Player p : players)
        {
            p.score = p.scopas;
        }

        playerWithMostCards().ifPresent(p -> p.score  = 1);
        playerWithMostSevens().ifPresent(p -> p.score  = 1);
        playerWithMostSpades().ifPresent(p -> p.score  = 1);
        playerWithSevenOfSpades().ifPresent(p -> p.score  = 1);
    }

Basically, I'm making a card game (called Scopa), and when updateScores() is called, the scores of every player should be updated. Players can earn a point by having the most cards, having the most sevens, or having the seven of spades. The logic for determining who has the most cards, sevens, and spades is very similar. How can I avoid repeating the logic across these three methods? Thank you.

CodePudding user response：

Another solution is just to make one function to calculate the scores. You're looping trough the player list 4 times and you're looping through the cards 3 times. You only need one loop for the players and one loop for the cards to check everything.

Something like this:

private void updateScores()
    {
        Player playerWithMostCards = null;
        Player playerWithMostSevens = null;
        Player playerWithMostSpades = null;
        int mostCards = 0;
        int mostSevens = 0;
        int mostSpades = 0;

        for(Player p : players)
        {
            // Most cards
            final int playerPileSize = p.pile.size();            

            if(playerPileSize > mostCards)
            {
              playerWithMostCards = p;
              mostCards = playerPileSize;
            }
            else if(playerPileSize == mostCards)
            {
              playerWithMostCards = null;
            }

            // NumberOfSevens and Spades
            int numberOfSevens = 0
            int numberOfSpades = 0

            for(Card c : p.pile)
            {
                if(c.is(Card.Value.SEVEN))
                {
                  numberOfSevens  ;
                }

                if(c.is(Card.Suit.SPADES))
                {
                    numberOfSpades  ;
                }
            }

            if (numberOfSevens > maxSevens) {
              playerWithMostSevens = p
              mostSevens = numberOfSevens              
            } else if (numberOfSevens == maxSevens) {
              playerWithMostSevens = null
            }

             if (numberOfSpades > maxSpades) {
              playerWithMostSpades = p
              mostSevens = numberOfSpades              
            } else if (numberOfSpades == maxSpades) {
              playerWithMostSpades = null
            }
        }

        playerWithMostCards.score  = 1;
        playerWithMostSevens.score  = 1;
        playerWithMostSpades.score  = 1;      

    }

I haven't tested this, but this shows the rough idea. You can practice by adding the last function.

CodePudding user response：

For the last two, you could make a function. I'm not familiar with the programming language that you're using (I don't even know which language it is- tag your question next time, please), but it would look something like this:

private Optional<Player> playerWithMostCards()
{
    Player scoringPlayer = null;
    int maxCount = 0;

    for(Player p : players)
    {
        final int count = p.pile.size();

        if(count > maxCount)
        {
            scoringPlayer = p;
            maxCount = count;
        }
        else if(count == maxCount)
        {
            scoringPlayer = null;
        }
    }

    return Optional.of(scoringPlayer);
}

private Optional<Player> playerWithMost([type] cvalue) // I don't know what the type of Card.[Value/Suit].[SEVEN/SPADES] is.
{
    Player scoringPlayer = null;
    int maxCount = 0;

    for(Player p : players)
    {
        int count = 0;

        for(Card c : p.pile)
        {
            if(c.is(cvalue))
            {
                count  ;
            }
        }

        if(count > maxCount)
        {
            scoringPlayer = p;
            maxCount = count;
        }
        else if(count == maxCount)
        {
            scoringPlayer = null;
        }
    }

    return Optional.of(scoringPlayer);
}
private Optional<Player> playerWithSevenOfSpades()
{
    for(Player p : players)
    {
        for(Card c : p.pile)
        {
            if(c == new Card("7S"))
            {
                return Optional.of(p);
            }
        }
    }

    return Optional.empty();
}

private void updateScores()
{
    for(Player p : players)
    {
        p.score = p.scopas;
    }

    playerWithMostCards().ifPresent(p -> p.score  = 1);
    playerWithMost(Card.Value.SEVEN).ifPresent(p -> p.score  = 1);
    playerWithMost(Card.Suit.SPADES).ifPresent(p -> p.score  = 1);
    playerWithSevenOfSpades().ifPresent(p -> p.score  = 1);
}

I'm not sure how to integrate the top one into the function.