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

• Thirupathi Thangavel: Updated question can be found here: codereview.stackexchange.com/questions/232662/…
• Thirupathi Thangavel: Repository: github.com/t-thirupathi/jaipur-board-game

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

• Thirupathi Thangavel: Thanks for the amazing answer. Great idea to have commodities as a separate class.
• Thirupathi Thangavel: Also using generator to cycle through users will be handy for board games with more than two players.
• RomanPerekhrest: @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.
• Maarten Fabré: 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
• Maarten Fabré: Giving the enum the cost as value won't work if 2 items cost the same: docs.python.org/3/library/…

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.

• Thirupathi Thangavel: Thanks for the valuable suggestions. I'm not used to oops concepts in python, so your answer was very helpful.