3
\$\begingroup\$

To help my son with his programming skills, I am trying refresh my limited Python knowledge. I chose a simple dice game called Pig Dice.

import sys
import random

HUMAN = 0
COMPUTER = 1
HOLD = False
ROLL = True


class Die:
    def __init__(self):
        self.value = random.randint(1, 6)

    def roll(self):
        self.value = random.randint(1, 6)

    @staticmethod
    def rolled_one():
        print("Rolled 1. \n")

    def __str__(self):
        return "Rolled " + str(self.value) + "."


class Box:
    def __init__(self):
        self.value = 0

    def reset_box(self):
        self.value = 0

    def add_dice_value(self, dice_value):
        self.value = self.value + dice_value

    def __str__(self):
        return "Box total:" + str(self.value)


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

    def add_score(self, player_score):
        self.score = self.score + player_score

    def set_name(self, name):
        self.name = name

    def __str__(self):
        return str(self.name) + ": " + str(self.score)


class ComputerPlayer(Player):
    def __init__(self):
        Player.__init__(self, name="ELIZA")

    @staticmethod
    def make_decision(box):
        while box.value < 20:
            print("I will roll again.")
            return ROLL
        print("I will hold.")
        return HOLD


class HumanPlayer(Player):
    def __init__(self):
        Player.__init__(self, name="HUMAN")

    @staticmethod
    def make_decision():
        human_decision = int(input("Roll again (Press 1) or Hold (Press 0)? "))
        if human_decision == 1:
            return ROLL
        else:
            return HOLD


class Score:
    def __init__(self, human, computer, box):
        self.human = human
        self.computer = computer
        self.box = box

    def __str__(self):
        return "Current Score -->   " + str(self.human) + "\t" + str(self.computer) + "\n"


class GameManager:
    def __init__(self):
        self.computer_player = ComputerPlayer()
        self.human_player = HumanPlayer()
        self.die = Die()
        self.box = Box()
        self.score = Score(self.human_player, self.computer_player, self.box)
        self.computer = 0
        self.turn = None
        self.action = None

    @staticmethod
    def welcome():
        print("*" * 70)
        print("Welcome to Pig Dice!" .center(70))
        print("*" * 70)
        print("The objective is to be the first to reach 100 points." .center(70))
        print("On each turn, the computer will roll a die." .center(70))
        print("The die value will stored in a temporary score box." .center(70))
        print("(If the die value is 1, the player earns no points," .center(70))
        print("and the turn switches to the other player.)" .center(70))
        print("The player has option to either roll again," .center(70))
        print("or hold. If you hold, the score in the" .center(70))
        print("temprory box will be assigned to you." .center(70))
        print("Good luck!" .center(70, "*"))
        print("Remember" .center(70, "*"))
        print("(Fortune favors the brave...." .center(70, "*"))
        print("(but chance favors the smart!" .center(70, "*"))
        print()
        print("I will now flip a coin to decide who starts" .center(70, " "))
        print()

    def decide_first_player(self):
        self.turn = random.randint(1, 2) % 2
        if self.turn == HUMAN:
            print("Human starts!" .center(70, " "))
        else:
            print("Eliza starts!" .center(70, " "))
        print()

    def switch_turns(self):
        if self.turn == COMPUTER:
            self.turn = HUMAN
        else:
            self.turn = COMPUTER

    def print_turn(self):
        if self.turn == HUMAN:
            print("Your turn.\n")
        else:
            print("My turn.\n")

    def check_score(self):
        if self.human_player.score > 99:
            print("Human wins!")
            sys.exit()
        elif self.computer_player.score > 99:
            print("Eliza win!")
            sys.exit()

    def assign_score(self):
        if self.turn == HUMAN:
            self.human_player.add_score(self.box.value)
        else:
            self.computer_player.add_score(self.box.value)

    def play_game(self):
        self.welcome()
        self.decide_first_player()
        while (self.human_player.score < 100) and (self.computer_player.score < 100):
            self.get_action()

    def get_action(self):
        self.action = ROLL
        self.box.reset_box()
        print(self.score)
        self.print_turn()
        while self.action == ROLL:
            self.keep_rolling()
        self.assign_score()
        self.check_score()
        self.switch_turns()

    def keep_rolling(self):
        self.die.roll()
        dice_value = self.die.value
        if self.turn == HUMAN:
            if dice_value == 1:
                self.die.rolled_one()
                self.box.reset_box()
                self.action = HOLD
                return
            else:
                self.box.add_dice_value(dice_value)
                print(self.die)
                print(self.box)
                self.action = self.human_player.make_decision()
                return
        else:
            if dice_value == 1:
                self.die.rolled_one()
                self.box.reset_box()
                self.action = HOLD
                return
            else:
                self.box.add_dice_value(dice_value)
                print(self.die)
                print(self.box)
                self.action = self.computer_player.make_decision(self.box)
                return


def main():
    game_manager = GameManager()
    game_manager.play_game()


main()

The code works, but a few matters worry me.

  1. I am not happy with the global variables. What design pattern would help me with those?

  2. On a related matter, I am having a difficulty on how to implement “player turn” in object oriented manner. Where does it belong? Whose property is it? (Because I did not deal with it properly, the get_action and keep_rolling methods stink.)

  3. On yet another related matter, the code is poor in the sense that it is not immediately scalable. For example, if there were more than one human players, I would have to rethink some of the methods.

  4. This one is due to my poor knowledge. the HumanPlayer and ComputerPlayer classes are inherited from the Player class. In assign_score method, I had to deal with each case separately; how can I do, say, assign_score to Player (whether human_player or computer_player)?

(Any other comment, criticism, suggestion is most welcome.)

\$\endgroup\$
5
\$\begingroup\$

Style review of current code

Reading up on PEP8 is never a bad thing. Mostly your code is clean, but there are several enhancements which can be made.

  • Add more space between methods – Methods (and functions) according to guidelines should have two new lines before them. This helps separating the methods from each other
  • Methods and classes should have docstrings – All methods and classes should have docstrings describing why they exist.
  • You mostly have good names – Most variables, classes and methods have names according to standard. The only deviation could arguably be that internal/private methods should be named _snake_case() something.
  • I'm not too fond of the welcome() print formatting, but this is a minor point, and in my code refactor I've just left it as is. You could have used multi-line texts, or lists of texts, and various other options.
  • Verify/validate input – When asking for input, it is often useful to verify that the user entered legal options. This can be done using an external method
  • Version compatible calling of parent constructor – I'm not sure if you're using Python 3 or Python 2, but a way to call the parent constructor in both version are to use super(HumanPlayer, self).__init__(name) where you replace the current class name instead of HumanPlayer.
  • Use var += 1 to increment variables – This looks a little neater than doubling the variable name

Code and class review

Previous was global comments, but here are some more localised to your choices of classes and code structure:

  • Drop the Score class – Let each player handle its own score, and then you can drop this entirely
  • Allow multiple players through a list of players – If you instead of two locked player variables, uses a list of players, you can pass how many human players and how many computer players to the constructor.
  • Let the turn / current player be index into player list – Instead of duplicating code, for the specialised player variables, you can let the current player be an index into the player list. This will simplify logic quite a bit. You can then start utilising the object structure, by simply adding scores to the current player, get the name of current player, and so on directly. You could take it all the way, and have a dedicated variable which holds the current player, but I chose to use the index
  • To change turns, add with modulo – To change turns, you can add one to the current player index, and take the modulo of number of players.
  • Try avoid one line methods – In most cases have a one line method makes it harder to understand code segments as you need to jump back and forth to see what that method actually did. If the code is simple and rather intuitive, avoid the one line method. If the method is needed as you use it in contexts like filtering, mapping or similar, they could be of use (but then again you can also use lambda's).

    This is a balance, where you need to balance between readability and maintainability. No black and white answers exists here, and you need to go a little bit by gut feeling, and not everyone will agree with choices we make

  • If possible avoid globals – In your case to define HOLD = False and ROLL = True. With good method names, it is much more intuitive to use the original values. Let keep_rolling() return True or False directly. The two others are eliminated as we now uses proper names for each player, and use the player list for access.
  • Alternative handling of rolling 1 – Instead of your logic, I opted for raising an exception when rolling 1. This can be a matter of taste, but I kind of felt that it a nice flow to it to use a raised RolledOneException.
  • Avoid set_variable(), when you keep it public anyway – It looked kind of strange to have Player.set_name(), when you can do Player.name = ... without violating guidelines. If you had chosen to have Player._name, and possibly had other side effects needed to be executed at the same time, you could have a set_name(), but as is was I thought it looked a little strange. I neither saw the need for all the __str__ you had defined, but your mileage may vary...
  • The if __name__ == '__main__': pattern – Adding this pattern before calling the main() allows for your code to be runned directly as a script, but also to be easily included as a module. I've used it the code to ask for how many humans and computers are to play, and could easily have read that from the arguments as well. And still, if used as a module, you could initiate the game from another script if you felt like it.

Code refactored

Here is the code when all of the above has been taken into account:

import random


def input_number(prompt='Please enter a number: ', minimum=0, maximum=None):
    """Read a positive number with the given prompt."""

    while True:
        try:
            number = int(input(prompt))
            if (number < minimum or
                (maximum is not None and number > maximum)):
                    print('Number is not within range: {} to {}'.format(minimum, maximum))
            else:
                break

        except ValueError:
            print('You need to enter a number')
            continue

    return number


class RolledOneException(Exception):
    pass


class Die:
    """A die to play with."""

    def __init__(self):
        self.value = random.randint(1, 6)

    def roll(self):
        """Returns the rolled dice, or raises RolledOneException if 1."""

        self.value = random.randint(1, 6)
        if self.value == 1:
            raise RolledOneException

        return self.value


    def __str__(self):
        return "Rolled " + str(self.value) + "."


class Box:
    """Temporary score box holder class."""

    def __init__(self):
        self.value = 0


    def reset(self):
        self.value = 0


    def add_dice_value(self, dice_value):
        self.value += dice_value


class Player(object):
    """Base class for different player types."""

    def __init__(self, name=None):
        self.name = name
        self.score = 0


    def add_score(self, player_score):
        """Adds player_score to total score."""

        self.score += player_score


    def __str__(self):
        """Returns player name and current score."""

        return str(self.name) + ": " + str(self.score)


class ComputerPlayer(Player):
    cpu_names=['Eliza', 'BigBlue', 'Foo', 'Bar']


    def __init__(self, number):
        """Assigns a cpu name from cpu_names, or Cpu#."""

        if number < len(self.cpu_names):
            name = self.cpu_names[number]
        else:
            name = 'Cpu{}'.format(number)

        super(ComputerPlayer, self).__init__(name)


    def keep_rolling(self, box):
        """Randomly decides if the CPU player will keep rolling."""

        while box.value < (10 + random.randint(1, 35)):
            print("  CPU will roll again.")
            return True
        print("  CPU will hold.")
        return False


class HumanPlayer(Player):
    def __init__(self, name):
        super(HumanPlayer, self).__init__(name)


    def keep_rolling(self, box):
        """Asks the human player, if they want to keep rolling."""

        human_decision = input_number("  1 - Roll again, 0 - Hold? ", 0, 1)
        if human_decision == 1:
            return True
        else:
            return False


class GameManager:
    def __init__(self, humans=1, computers=1):
        """Initialises the game, optionally asking for human player names."""

        self.players = []
        if humans == 1:
            self.players.append(HumanPlayer('Human'))
        else:
            for i in range(humans):
                player_name = input('Enter name of human player no. {}: '.format(i))
                self.players.append(HumanPlayer(player_name))

        for i in range(computers):
            self.players.append(ComputerPlayer(i))

        self.no_of_players = len(self.players)

        self.die = Die()
        self.box = Box()


    @staticmethod
    def welcome():
        """Prints a welcome message including rules."""

        print("*" * 70)
        print("Welcome to Pig Dice!" .center(70))
        print("*" * 70)
        print("The objective is to be the first to reach 100 points." .center(70))
        print("On each turn, the player will roll a die." .center(70))
        print("The die value will stored in a temporary score box." .center(70))
        print("(If the die value is 1, the player earns no points," .center(70))
        print("and the turn goes to the next player.)" .center(70))
        print("A human player has an option to either roll again," .center(70))
        print("or hold. If you hold, the score in the" .center(70))
        print("temporary box will be added to your total score." .center(70))
        print(" Good luck! " .center(70, "*"))
        print(" Remember " .center(70, "*"))
        print(" Fortune favors the brave... " .center(70, "*"))
        print(" but chance favors the smart! " .center(70, "*"))
        print()
        print("I will now decide who starts" .center(70, " "))
        print()


    def decide_first_player(self):
        """Randomly chooses a player to begin, and prints who is starting."""

        self.current_player = random.randint(1, self.no_of_players) % self.no_of_players

        print('{} starts'.format(self.players[self.current_player].name))


    def next_player(self):
        """Advanced self.current_player to next player."""
        self.current_player = (self.current_player + 1) % self.no_of_players



    def previous_player(self):
        """Changes self.current_player to previous player."""

        self.current_player = (self.current_player - 1) % self.no_of_players


    def get_all_scores(self):
        """Returns a join all players scores."""

        return ', '.join(str(player) for player in self.players)


    def play_game(self):
        """Plays an entire game."""

        self.welcome()
        self.decide_first_player()

        while all(player.score < 100 for player in self.players):
            print('\nCurrent score --> {}'.format(self.get_all_scores()))
            print('\n*** {} to play ***'.format(self.players[self.current_player].name))
            self.box.reset()

            while self.keep_rolling():
                pass

            self.players[self.current_player].add_score(self.box.value)
            self.next_player()

        ## The previous player has won...
        self.previous_player()
        print(' {} has won '.format(self.players[self.current_player].name).center(70, '*'))


    def keep_rolling(self):
        """Adds rolled dice to box. Returns if human/cpu wants to continue.

        If either player rolls a 1, the box value is reset, and turn ends.
        """
        try:
            dice_value = self.die.roll()
            self.box.add_dice_value(dice_value)
            print('Last roll: {}, new box value: {}'.format(dice_value, self.box.value))

            # Check if human (by asking) or computer(calculating) will keep rolling
            return self.players[self.current_player].keep_rolling(self.box)

        except RolledOneException:
            print('  Rolled one. Switching turns')
            self.box.reset()
            return False


def main():
    human_players = input_number('How many human players? ')
    computer_players = input_number('How many computer players? ')

    game_manager = GameManager(human_players, computer_players)
    game_manager.play_game()


if __name__ == '__main__':
    main()

Hope you learned something along the way!

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for sparing your time. Once I digest the code, and with your permission, I may have to ask few more questions with respect to your edition. \$\endgroup\$ – blackened Nov 17 '15 at 10:25
  • \$\begingroup\$ 1) Suppose there are multiple players, and I want to add this to ComputerPlayer’s decision algorithm: “If any of the opponent’s score is more than some arbitrary score, then do this and that. 2) Suppose I want to perform a simulation testing various strategies, should I implement Strategy Pattern for that? 3) Minor point: According to PEP8, shouldn’t there be one new line between methods? 4) With respect to keeping score: Isn’t it more natural to have player’s have a score attribute, and also have a separate Score class? (Is it not the case that the game has a “score” object? \$\endgroup\$ – blackened Nov 18 '15 at 20:04
  • \$\begingroup\$ Re 1) Then send the list of players into make_decision and traverse it from there. Re 2), don't understand what you mean. Re 3), PEP8 does imply two new lines between methods. Re 4) Whilst the score is only a single number there is no need for it be a separate class, just let it be an attribute of the player. Why do you want a score object? What's the purpose of it? \$\endgroup\$ – holroy Nov 18 '15 at 20:09

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.