Implementing simplified Jaipur board game

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()


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()

• Thanks for the amazing answer. Great idea to have commodities as a separate class. Oct 23, 2019 at 13:13
• Also using generator to cycle through users will be handy for board games with more than two players. Oct 23, 2019 at 13:14
• @ThirupathiThangavel, you're welcome. I've tried to provide a comprehensive solution/refactoring, it's great if people would find it useful for their needs. Oct 23, 2019 at 13:14
• Nice answer. On the enums, I would not keep the values of the commodities in the market, but the actual commodities. If you change the player.hand to a collections.Counter, the only thing you need to change is def hand_size(self): return sum(self.hand.values()) and in Player.sell: if commodity is None: commodity=self.hand.most_common(1)[0], but the rest of the code becomes more expressive. You could even assign the cost of the item to the value of the enum Oct 23, 2019 at 14:29
• Giving the enum the cost as value won't work if 2 items cost the same: docs.python.org/3/library/… Oct 24, 2019 at 7:27

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.

• Thanks for the valuable suggestions. I'm not used to oops concepts in python, so your answer was very helpful. Oct 23, 2019 at 13:07