11
\$\begingroup\$

I've implemented the advice given here and elsewhere as best as I understand: Ten Thousand: a dice game for multiple players

I've added a computer player as well. Help me make it even better. What is incorrect, superfluous, or not as Pythonic as it could be? Are there statements or methods I'm obviously unaware of? Is my use of classes appropriate, or could I still move, say, set_player() out of the Game class?

import random
import collections
import numpy as np

msg = ('''
                    Welcome to Ten Thousand
                           The Game!
Objective:
    Score 10,000 points.
    Any turn in which a player ends with more than 10,000 points will be the 
    final turn.
    If multiple players finish with more than 10,000 points, the winner is the
    player with the most points.

To score:
    1's and 5's are worth 100 and 50 points respectively.
    Three-of-a-kind is worth the die number x 100. (Three 1's is 1000 points)
    Four-of-a-kind is worth double the same Three-of-a-kind.
    Five-of-a-kind is double Four-of-a-kind.
    Six of any number is worth 5000 points.
    A six dice straight is worth 1500 points.
    Three pairs are also worth 1500 points.

To play:
    Your dice will appear in [brackets].
    Choose your dice by the reference number located above your dice.
    You must score 500 points to get on the board.
    You'll press Enter a lot. Sorry about that. There will be graphics soon.
    Try and break things! If you do, please tell my how you did it.
    Screen shots of the error message are especially helpful.
    Have fun and thanks for helping me develop my first app!!
''')

valuedict = {1:
                {
                0: 0,
                1: 100,
                2: 200,
                3: 1000,
                4: 2000,
                5: 4000,
                6: 5000
                },
            2: {
                0: 0,
                1: 0,
                2: 0,
                3: 200,
                4: 400,
                5: 800,
                6: 5000
                },
            3: {
                0: 0,
                1: 0,
                2: 0,
                3: 300,
                4: 600,
                5: 1200,
                6: 5000
                },
            4: {
                0: 0,
                1: 0,
                2: 0,
                3: 400,
                4: 800,
                5: 1600,
                6: 5000
                },
            5: {
                0: 0,
                1: 50,
                2: 100,
                3: 500,
                4: 1000,
                5: 2000,
                6: 5000
                },
            6: {
                0: 0,
                1: 0,
                2: 0,
                3: 600,
                4: 1200,
                5: 2400,
                6: 5000
                }
                }


class Game:
    def __init__(self, player_list):
        self.player_list = player_list

    def set_player(self, player_list=[]):
        '''Sets number of players and player names.'''
        players = int(input('How many humans are playing?''\n', ))
        digi_player = input('Do you want to play against Digital Overlord? y/n''\n')
        if digi_player == r'y':
            name = 'Digital Overlord'
            self.name = ComPlayer(name)
            player_list.append(self.name)
        x = 0
        while x < players:
            name = input(f'Player {x + 1}, Enter your name:''\n', )
            self.name = Player(name)
            player_list.append(self.name)
            x += 1
        return player_list


class Player:
    def __init__(self, name, total_score=0):
        self.total_score = total_score
        self.name = name

    def pick(self, roll):
        '''Takes user input to choose dice by index. Returns choices.'''
        choice_list = []
        try:
            while True:
                choose = int(input(
'''Choose which die to keep by position 1-6
Type position number, then enter. Repeat for all choices.
Press enter when finished
'''
                            ))-1
                if choose >= len(roll):
                    print(f'\n{choose} not available\n')
                    continue
                if choose in choice_list:
                    print('\nYou can only pick a die once.\nNo cheating!')
                    continue
                else:
                    choice_list.append(choose)
        except ValueError:
            choice = [roll[x] for x in choice_list]
            False
        if choice is not None:
            counts = collections.Counter(choice)
            for _ in counts.items():
                if valuedict[_[0]][_[1]] == 0 and is_full_house(choice) == 0 \
                        and is_straight(choice) == 0:
                    print(f'\n{_[0]} is not a keeper.\nNo cheating!')
                    choice = None
        if choice is None:
            choice = []
        return choice


class ComPlayer:
    def __init__(self, name, total_score=0):
        self.total_score = total_score
        self.name = name

    def pick(self, roll):
        '''Allows computer to choose dice. Boop beep.'''
        counts = collections.Counter(roll)
        keepers = np.zeros(6, dtype=int)
        if is_full_house(roll) > sum(valuedict[die][count] for die, count
                    in counts.items()):
            keepers = list(roll)
        elif is_straight(roll) == 1500:
            keepers = list(roll)
        else:
            for _ in counts.items():
                if valuedict[_[0]][_[1]] > 0:
                    for x in range(len(roll)):
                        if roll[x] == _[0]:
                            keepers = np.insert(keepers, x, _[0])
                            keepers = np.delete(keepers, x + 1)
            while 0 in keepers:
                try:
                    for y in range(len(keepers)):
                        if keepers[y] == 0:
                            keepers = np.delete(keepers, y)
                except IndexError:
                    pass
        choice = list(keepers)
        if choice == []:
            return []
        return choice

def is_full_house(choice):
    if choice is None:
        return 0
    else:
        if len(choice) == 6 and\
                    all(a == b for a, b in zip(*[iter(sorted(choice))]*2)):
            return 1500
        else:
            return 0

def is_straight(choice):
    if choice is None:
        return 0
    else:
        if sorted(choice) == list(range(1, 7)):
            return 1500
        else:
            return 0

def keep_score(choice):
    '''Scores choices from self.pick().'''
    score = 0
    counts = collections.Counter(choice)
    score = sum(valuedict[die][count] for die, count in counts.items())
    straight = is_straight(choice)
    full_house = is_full_house(choice)
    if score < straight or full_house:
        score = max(straight, full_house)
    if score == 0:
        print('\nNo keepers. What a bummer.\nYour score for this round is: 0')
        return 0
    return score

def throw(keepers_list):
    '''Rolls dice and returns rolls in a list.'''
    roll = [random.randint(1, 6) for _ in range(6-len(keepers_list))]
    return roll

def turn(player):
    '''One player turn.'''
    round_score = 0
    keepers_list = []
    throw_count = 0
    while True:
        roll = throw(keepers_list)
        print(f'\n{player.name}, your dice in []\n 1  2  3  4  5  6\n{roll}\n')
        choice = player.pick(roll)
        print(f'\nHere are your choices: {choice}')
        score = keep_score(choice)
        if score == 0:
            return False
        print(f'\nScore for this throw is: {score}')
        round_score += score
        print(f'\nTotal score for this turn is: {round_score}')
        keepers_list += choice
        if len(keepers_list) == 6:
            print('\nSix keepers! Roll \'em again!')
            keepers_list = []
            throw_count = -1
        if player.name != 'Digital Overlord':
            again = input('''
Roll again or keep?
Enter = roll K = keep'''
                    )
            if again == r'k':
                player.total_score += round_score
                if player.total_score < 500:
                    print('\nMust score at least 500 to get on the board.')
                    player.total_score = 0
                    round_score = 0
                    return False
                else:
                    print(f'\n{player.name}\'s score this turn: {round_score}')
                    return False
            else:
                throw_count += 1
        else:
            if round_score >= 500 and len(keepers_list) > 2:
                player.total_score += round_score
                print(f'\n{player.name}\'s score this turn: {round_score}')
                return False
            else:
                throw_count += 1
    return False

def take_turns(player_list):
    '''Sets winning score, switches between players'''
    winner_dict = {}
    while not any(player.total_score >= 10000 for player in player_list):
        for player in player_list:
            turn(player)
            for x in range(len(player_list)):
                print(f'\n{player_list[x].name}\'s total score is: '
                      f'{player_list[x].total_score}')
    for x in range(len(player_list)):
        winner_dict.update({player_list[x].total_score: player_list[x]})
    winner = winner_dict[max(winner_dict.keys())]
    False
    print(f'\nThe winner is {winner.name}, '
          f'with a score of: {winner.total_score}!!')
    return False

def main():
    player_list = Game.set_player(Game, [])
    take_turns(player_list)

print(msg)
if __name__=="__main__":
    main()
\$\endgroup\$
  • \$\begingroup\$ Why did you not change is_full_house to return a boolean? Among other changes? \$\endgroup\$ – Peilonrayz Feb 15 '18 at 9:14
  • \$\begingroup\$ Because some combinations can be scored as a full-house or as four-of-a-kind + a pair. The greater of the scores should be used. \$\endgroup\$ – Paul K Feb 15 '18 at 9:16
  • \$\begingroup\$ you can still do that, even if you return a boolean. \$\endgroup\$ – Peilonrayz Feb 15 '18 at 9:17
  • \$\begingroup\$ I'll work on that. \$\endgroup\$ – Paul K Feb 15 '18 at 9:18
  • \$\begingroup\$ Why would you downvote? \$\endgroup\$ – Paul K Feb 15 '18 at 9:29
5
\$\begingroup\$
  • Rules note: The rulesheet doesn't describe the "Six keepers roll again" rule that's in the code.

  • import numpy as np
    

    I don't think numpy is necessary. There are far easier ways to manipulate lists built-in to the language.

  • msg = ('''
    

    No need to wrap in parentheses (they don't do anything in this context)

  • valuedict =
    

    valuedict is a horrible variable name for the variable that contains the scoring rules. How about scoring_rules_table? valuedict does not explain what the variable contains or what it is used for. Putting "dict" in the variable name doesn't help anybody understand anything; its type will be obvious from how it's used.

  • valuedict = {1:
                    {
                    0: 0,
                    1: 100,
                    2: 200,
                    3: 1000,
                    4: 2000,
                    5: 4000,
                    6: 5000
                    },
                2: {
    

    Also, I question the choice of implementing this table as a dict of dicts. I think it would be much better to implement it as a list of lists (or list of array("I",..)s from the array module):

    scoring_rules = [ [0, 100, 200, 1000, 2000, 4000, 5000], [ ... ], ... ]
    

    Since you're using integers to index into it, a list will work, and having a dict just costs you sped. Or, since each sublist is the same size, implement it as a 2-dimensional numpy.ndarray.

    scoring_rules = np.array([[0, 100, 200,...],[...]], np.int32)
    
  • Thirdly, a big data table like this should not be part of your code, it should be a separate file, either a python file that gets imported at compiletime, or read from disk (e.g. from a csv file) at runtime using a module.

  • class Game:
    

    There's no docstring here, so I have no idea what the class is used for. The purpose of using classes is to group together related data and/or functions. Your Game class doesn't seem to store any data (the __init__() is never called, and the member it stores into is never used anywhere). A Game class should store data about the overall state of the game, for instance a list of players and whose turn it is, and have methods that use and manipulate the state of the whole game. A player list would be one of its members.

  •     def __init__(self, player_list):
            self.player_list = player_list
    

    This function is never called, and the member variable it assigns is never used. Why does it exist?

  •     def set_player(self, player_list=[]):
    

    The player list is an aspect of the game so it should be a member variable (accessed via self.), not a parameter.

  •         '''Sets number of players and player names.'''
            players = int(input('How many humans are playing?''\n', ))
    

    This will exit on an exception if the user does not enter an integer. Guard it with try/except

  •         digi_player = input('Do you want to play against Digital Overlord? y/n''\n')
            if digi_player == r'y':
    

    What if the user types Y ? or some variation of yes?

  •             name = 'Digital Overlord'
                self.name = ComPlayer(name)
                player_list.append(self.name)
    

    What's the purpose of the self.name member variable? Why isn't it a local? Do you need it to persist?

  •         x = 0
            while x < players:
                name = input(f'Player {x + 1}, Enter your name:''\n', )
                self.name = Player(name)
                player_list.append(self.name)
                x += 1
    

    Use for x in range(1, players+1). Also again what is the purpose of self.name? It's not even a name, it's a Player object instance.

  • class Player:
    

    Need a docstring here too.

  •     def __init__(self, name, total_score=0):
            self.total_score = total_score
            self.name = name
    

    When would you ever create a new player with a nonzero total score?

  •     def pick(self, roll):
            '''Takes user input to choose dice by index. Returns choices.'''
    

    Needs a more descriptive name. The docstring needs to be clearer.

  •         choice_list = []
    

    Order doesn't matter so it should be a set

  •         try:
                while True:
                    choose = int(input(
    

    Exceptions should be for exceptional situations, not the normal way of exiting an infinite loop. You should create a new function that takes the string, checks if it's an integer, and then returns the integer or None. Then have checking the return value be the condition of your loop. You may want to call this function before the loop as well as in the loop, or use the "loop-and-a-half" method.

  •                 if choose in choice_list:
                        print('\nYou can only pick a die once.\nNo cheating!')
    

    Unnecessarily hostile; people do make typos you know.

  •         except ValueError:
                choice = [roll[x] for x in choice_list]
                False
    

    And the except block should not have program logic, it should only recover from the exception. What's the False doing there?

  •         if choice is not None:
    

    This if will always be taken; choice will never be None. If a list comprehension doesn't end up with any elements you get the empty list. But your code still works correctly, so what's the point of the if?

  •             for _ in counts.items():
    

    The _ variable should only be used for something that doesn't matter. It should not be the actual loop variable if you're going to use it.

  •                 if valuedict[_[0]][_[1]] == 0 and is_full_house(choice) == 0 \
                            and is_straight(choice) == 0:
                        print(f'\n{_[0]} is not a keeper.\nNo cheating!')
                        choice = None
    

    Again this is needlessly punitive. Make one mistake and lose your turn? But even if that's what you intend, why continue with the rest of the loop? Just break immediately.

  •         if choice is None:
                choice = []
    

    What's the difference between how your code treats None and [] that you need to use both of them?

  • class ComPlayer:
    

    Either ComPlayer should be a subclass of Player, or ComPlayer and HumanPlayer should both be subclasses of Player. Eliminate the duplicated code!

  •                             keepers = np.insert(keepers, x, _[0])
                                keepers = np.delete(keepers, x + 1)
    

    Just use normal list manipulation. Instead of an insert and a delete just change the item. If you really need to insert or delete in the middle of a list use regular slicing.

  •                 try:
                        for y in range(len(keepers)):
                            if keepers[y] == 0:
                                keepers = np.delete(keepers, y)
    

    If you ever are going to loop over a list and do something to each element, there's a better way to do it than using a for loop. Also this code doesn't work if there are two consecutive 0 elements. Much simpler to just do [y for y in keepers if y != 0]

  •         if choice == []:
                return []
            return choice
    

    Can you think of a shorter way to accomplish this?

  • def is_full_house(choice):
    

    These types of functions are game rules and should be members of Game

        if choice is None:
            return 0
        else:
            if len(choice) == 6 and\
    

    When is len(choice) going to be something other than 6?

  •                     all(a == b for a, b in zip(*[iter(sorted(choice))]*2)):
    

    Good call sorting it. Bad call iterating over the whole list twice. Just do two comparisons after the sort. I don't think this code actually works.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you! Makes me feel like my "beginner" tag was prematurely removed. Haha. I'll be looking into each of these excellent points. Much appreciated! \$\endgroup\$ – Paul K Feb 16 '18 at 23:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.