Implementing simplified Jaipur board game

Question

I wanted to build a bot for playing Jaipur board game. I started by implementing a simplified version of the game without bot. Want to make sure I'm going in the right direction. Few of the simplifications from the actual game:

• Prices of commodities are fixed
• No camels
• Only one card is visible and can be picked from the market
• Cards are not shuffled at the start

Code:

import random
import numpy as np

DIAMOND = 0
GOLD = 1
SILVER = 2
SILK = 3
SPICE = 4
LEATHER = 5
commodities = [DIAMOND, GOLD, SILVER, SILK, SPICE, LEATHER]

price = [7, 6, 5, 3, 3, 1] # DIAMOND, GOLD, SILVER, SILK, SPICE, LEATHER

TAKE = 0
SELL = 1
actions = [TAKE, SELL]


class Jaipur():
    def __init__(self, player1, player2):
        self.market = [DIAMOND] * 6 + [GOLD] * 6 + [SILVER] * 6 + [SILK] * 8 + [SPICE] * 8 + [LEATHER] * 10

        player1 = globals()[player1]
        player2 = globals()[player2]

        self.player1 = player1(tag='P1')
        self.player2 = player2(tag='P2')

        self.winner = None
        self.player_turn = self.player1

    def play_game(self):

        # while len(self.market != 0):
        while self.winner is None:
            self.state = self.play_move()
            self.game_winner()

            if self.winner is not None:
                print('P1 score: ', self.player1.score)
                print('P2 score: ', self.player2.score)

                print('Winner is ', self.winner)

    def play_move(self, learn=False):

        if self.player_turn == self.player1:
            self.print_game()
            new_market = self.player1.make_move(self.market, self.winner)
            self.player_turn = self.player2

        elif self.player_turn == self.player2:
            self.print_game()
            new_market = self.player2.make_move(self.market, self.winner)
            self.player_turn = self.player1

    def print_game(self):
        print(self.market)
        print('turn: ', self.player_turn.tag)
        print('market: ', self.market)
        print('player hand: ', self.player_turn.hand)
        print('player score: ', self.player_turn.score)

    def game_winner(self):

        if len(self.market) == 0:
            if self.player1.score > self.player2.score:
                self.winner = self.player1.tag
            else:
                self.winner = self.player2.tag
        return self.winner

class Player():
    def __init__(self, tag):
        self.tag = tag
        self.hand = [0] * len(commodities)
        self.score = 0

    def hand_size(self):
        return sum(self.hand)

    def take(self, market):
        print('taking..')
        if self.hand_size() == 7:
            return market

        if len(market) == 0:
            return market

        taken = market.pop()
        self.hand[taken] += 1

        return market

    def sell(self, market, commodity=None):
        print('selling..')
        if commodity is None:
            commodity = np.argmax(self.hand)

        if commodity in [DIAMOND, GOLD, SILVER] and self.hand[commodity] < 2:
            return market

        if self.hand[commodity] < 1:
            return market

        self.score += self.hand[commodity] * price[commodity]
        if self.hand[commodity] == 3:
            self.score += 2
        elif self.hand[commodity] == 4:
            self.score += 5
        elif self.hand[commodity] >= 5:
            self.score += 9

        self.hand[commodity] = 0

        return market

    def make_move(self, market, winner):
        # move = int(input('0: Take, 1: Sell. Choose move..'))
        move = random.randint(0, 1)
        new_market = market
        if move == 0:
            new_market = self.take(market)
        elif move == 1:
            new_market = self.sell(market)

        return new_market


def play():
    game = Jaipur('Player', 'Player')
    game.play_game()

play()

Answer 1

Complex refactorings and optimizations:

Relation
When starting restructuring the initial program we need to reconsider the relation between game (Jaipur) and players.
The current approach tries to instantiate players from global scope by class name:

player1 = globals()[player1]
player2 = globals()[player2]

which is definitely a bad way. Instead, let's pass players names/tags to the game constructor:

game = Jaipur('P1', 'P2')

We are doing so to internally create Player instances and pass back-reference to the same game Jaipur instance for each player. Thus, each player can access/request the needed features/behavior from game's public interface.
When looking at Players all crucial methods take(self, market), sell(self, market, commodity=None), make_move(self, market, winner) we see that they all expect market and return that market although all the callers aren't using that return value.
But the market is owned by the game. So we make a player to request a copy of the current market state from the game.

---

The commodities
As was mentioned in previous answer, the commodity list presented as consecutive integer numbers is a good candidate to be enumeration.
But we'll go further and apply enum.unique decorator that ensures only one name is bound to any one value. To proceed with new enum let's look at Player's sell method and its condition:

if commodity in [DIAMOND, GOLD, SILVER] ...

it checks if a particular commodity is the most costly one.
We'll give such a responsibility to our Commodity enum class so eventually it'll look as below:

@unique
class Commodity(Enum):
    DIAMOND = 0
    GOLD = 1
    SILVER = 2
    SILK = 3
    SPICE = 4
    LEATHER = 5

    @classmethod
    def is_costly(cls, val):
        return val in [cls.DIAMOND.value, cls.GOLD.value, cls.SILVER.value]

We'll set the current prices for commodities as a constant attribute of the game:

C_PRICES = [7, 6, 5, 3, 3, 1]  # DIAMOND, GOLD, SILVER, SILK, SPICE, LEATHER

---

Jaipur (game) class refactoring:

• play_game method. The crucial while loop with condition self.winner is None inefficiently checks for if self.winner is not None: on each iteration.
  Instead, we'll apply a convenient Python's feature while ... else

• play_move method. Essentially initiates the current player action (take or sell) and switches/sets to another player. This is a good case for itertools.cycle feature (to infinitely switch to next player) cycle([self.player1, self.player2]). See the implementation in bottom full code.

• finding price for a commodity (initially based on global list access price[commodity]) is now moved to a game class:

  @classmethod
  def get_price(cls, commodity):
      return cls.C_PRICES[commodity]
  

• game class is able to pick a commodity from the market for a player by request:

  def pick_commodity(self):
      return self._market.pop()
  

---

Player class refactoring:

• the constructor is now also accepts game parameter (Jaipur(game) instance) as a reference for the current game the player plays in.

  def __init__(self, tag, game)
  

• take method. It tells that the player can only pick a commodity if he hasn't already took 7 ones (if self.hand_size() == 7) or he can't pick from the empty market if len(market) == 0. That's a sign for Consolidate conditional refactoring technique.

• sell method. np.argmax(self.hand) is aimed to return a list of indices of maximum value. Instead we'll return a position/index of the most frequent commodity in player's hand: self.hand.index(max(self.hand)).
  The method also tells that the player can not sell the costly commodity of one if commodity in [DIAMOND, GOLD, SILVER] and self.hand[commodity] < 2 and can't sell an empty commodity if self.hand[commodity] < 1. That's also a sign for Consolidate conditional refactoring technique. See below.

• make_move(self, market, winner) method. Accepts/passes/assigns and returns new_market though it's not used neither by caller or the method itself. This method would be significantly simplified/optimized.

---

From theory to practice, the final version:

import random
from enum import Enum, unique
from itertools import cycle


@unique
class Commodity(Enum):
    DIAMOND = 0
    GOLD = 1
    SILVER = 2
    SILK = 3
    SPICE = 4
    LEATHER = 5

    @classmethod
    def is_costly(cls, val):
        return val in [cls.DIAMOND.value, cls.GOLD.value, cls.SILVER.value]


class Jaipur:
    C_PRICES = [7, 6, 5, 3, 3, 1]  # DIAMOND, GOLD, SILVER, SILK, SPICE, LEATHER

    def __init__(self, player1_tag, player2_tag):
        self._market = [Commodity.DIAMOND.value] * 6 + [Commodity.GOLD.value] * 6 + [Commodity.SILVER.value] * 6 + \
                       [Commodity.SILK.value] * 8 + [Commodity.SPICE.value] * 8 + [Commodity.LEATHER.value] * 10

        self.player1 = Player(tag=player1_tag, game=self)
        self.player2 = Player(tag=player2_tag, game=self)

        self.winner = None
        self._players_gen = cycle([self.player1, self.player2])  # cycling `players` generator
        self.player_turn = next(self._players_gen)

    @property
    def market(self):
        return self._market.copy()

    @classmethod
    def get_price(cls, commodity):
        return cls.C_PRICES[commodity]

    def pick_commodity(self):
        return self._market.pop()

    def play_game(self):
        while self.winner is None:
            self.switch_player()
            self.game_winner()
        else:
            print('P1 score:', self.player1.score)
            print('P2 score:', self.player2.score)
            print('Winner is', self.winner)

    def switch_player(self, learn=False):
        self.player_turn.make_move()
        self.player_turn = next(self._players_gen)
        self.print_game()

    def print_game(self):
        print('turn: ', self.player_turn.tag)
        print('_market: ', self._market)
        print('player hand: ', self.player_turn.hand)
        print('player score: ', self.player_turn.score)

    def game_winner(self):
        if len(self._market) == 0:
            if self.player1.score > self.player2.score:
                self.winner = self.player1.tag
            else:
                self.winner = self.player2.tag
        return self.winner


class Player:
    def __init__(self, tag, game):
        self.tag = tag
        self.hand = [0] * len(Commodity)
        self.score = 0

        self._game = game

    def hand_size(self):
        return sum(self.hand)

    def take(self):
        print('taking..')
        if len(self._game.market) > 0 and self.hand_size() < 7:
            taken = self._game.pick_commodity()
            self.hand[taken] += 1

    def sell(self, commodity=None):
        print('selling..')
        if commodity is None:
            commodity = self.hand.index(max(self.hand))

        if (Commodity.is_costly(commodity) and self.hand[commodity] > 1) or self.hand[commodity] > 0:
            self.score += self.hand[commodity] * Jaipur.get_price(commodity)

            if self.hand[commodity] == 3:
                self.score += 2
            elif self.hand[commodity] == 4:
                self.score += 5
            elif self.hand[commodity] >= 5:
                self.score += 9

            self.hand[commodity] = 0

    def make_move(self):
        # move = int(input('0: Take, 1: Sell. Choose move..'))
        move = random.randint(0, 1)
        self.take() if move == 0 else self.sell()


def play():
    game = Jaipur('P1', 'P2')
    game.play_game()


play()

Answer 2

Here are a couple of first-glance observations:

1. Use enum. If you are going to have all-caps names with integers, you might as well import the module and get all the benefits.

2. Don't pass classes by name. Pass them by value. Change this: game = Jaipur('Player', 'Player') into something like game = Jaipur(Player, Player) (note: no quotes).

   Classes are first-class objects in Python. You can pass them around just like any other object.

3. Use the game class to enforce the rules. You have code in your Player class to check on things like the minimum number of commodities to trade. Put that in the game class instead. Also, make the game class the keeper of records. If your Player class is supposed to be an AI, then let the game handle the record-keeping and have the player just tell the game what it wants. (An example of the Single Responsibility Principle.)

4. Don't import numpy just to use argmax! Python defines a max() builtin.