3
\$\begingroup\$

I developed a Chinese Poker game for better understanding classes in Python. It is not finished yet, but the main part is.

The game mechanics:

  • There is 1 board for each player. Each board have 3 rows.

  • The first row can receive up to 3 cards, and the other 2, 5 cards each. A total of 13 cards in each board

  • First, each player receive 5 cards and then place then within the boards. Player1 place all his cards, and then Player2 place all his cards.

  • After that, each player receive 3 cards per turn. Place 2 cards in the boards, and discard the last one. Cards are dealed until the board is full.

My code executes them and the score system is the next step. I'd like you to evaluate my code and point suggestions to improve it.

from random import shuffle

def turn_str(n):
    return '%d' % n

deck =  (map(turn_str, range(2,11)) + ['J', 'Q', 'K', 'A']) * 4
deck.sort()

naipes = ['c', 'h', 's', 'd'] * 13

def add_suits(x, y):
    return x + y

full_deck = map(add_suits, deck, naipes)
shuffle(full_deck)

class User(object):
    def __init__ (self, name):
        self.name = name
        self.board = [[],[],[]]
        self.hand = []

    def receive_cards(self, number_of_cards):
        self.hand = full_deck[0:number_of_cards]
        del full_deck[0:number_of_cards]

    def board_limit(self, board_number):
        if board_number == 1:
            if len(self.board[board_number - 1]) < 3:
                return True
            else:
                return False
        if board_number == 2 or board_number == 3:
            if len(self.board[board_number - 1]) < 5:
                return True
            else:
                return False

    def place_first_cards(self):
        print "Your time to play %s. Here is your hand. Pick a card and choose a row to place it" % self.name
        while len(self.hand) != 0:
            print '-'.join(self.hand)
            for i in self.board:
                print i
            card_placed = int(raw_input("Choose a card (1 - %d): " % len(self.hand)))
            chosen_board = int(raw_input("Choose a board (1-3): "))
            if self.board_limit(chosen_board):
                self.board[chosen_board - 1].append(self.hand[card_placed - 1])
                self.hand.pop(card_placed - 1)
            else: 
                print "The board is full. Choose another one."

    def place_cards(self):
        print "Your time to play %s. Here is your hand. Pick a card and choose a row to place it" % self.name
        while len(self.hand) != 1:
            print '-'.join(self.hand)
            for i in self.board:
                print i
            card_placed = int(raw_input("Choose a card (1 - %d): " % len(self.hand)))
            chosen_board = int(raw_input("Choose a board (1-3): "))
            if self.board_limit(chosen_board):
                self.board[chosen_board - 1].append(self.hand[card_placed - 1])
                self.hand.pop(card_placed - 1)
            else:
                print "The board is full. Choose another one."
        self.hand.pop(0)


player1 = raw_input("Player 1, type your nickname: ")
player2 = raw_input("Player 2, type your nickname: ")

user1 = User(player1)
user2 = User(player2)

print "Welcome. %s you will play against %s. Good luck for both of you." % (user1.name, user2.name)

user1.receive_cards(5)
user2.receive_cards(5)

user1.place_first_cards()
user2.place_first_cards()

user1.receive_cards(3)
user2.receive_cards(3)

user1.place_cards()
user2.place_cards()

user1.receive_cards(3)
user2.receive_cards(3)

user1.place_cards()
user2.place_cards()

user1.receive_cards(3)
user2.receive_cards(3)

user1.place_cards()
user2.place_cards()

user1.receive_cards(3)
user2.receive_cards(3)

print user1.board
print user2.board
\$\endgroup\$
3
\$\begingroup\$

Repeat actions

Look at the bottom of your code. This is the layout:

do something with user1
do the same thing with user2

do something with user1
do the same thing with user2

...

You have a lot of repeated code down there. To reduce this repetition, you should store the two players in an array, and then each time loop through the array an do what you need to do with each player.

To make it cleaner, you could use comprehensions:

user.receive_cards(5)    for user in users
user.place_first_cards() for user in users
user.receive_cards(3)    for user in users
...

This clearly illustrates each step of the game play, and doesn't have a repetitive line right after each one.

In fact, you could simplify this even further. If you keep a list of lambdas that show/do each of the different game moves (receive_cards, play_first_cards, etc.), then you easily loop through the list, calling each lambda.

Here's what the list might look like:

game_moves = [lambda user: user.receive_cards(5), lambda user: user.place_first_cards(), ...]

Then:

for game_move in game_moves:
    game_move(for user in users)

Note: I didn't have time to test this out, so this may not work. Leave a comment if there is an issue; I'll fix it later.

Now, with the list, each part of the game item by item is easy visible just a glance at the list.


If this, return true. Else that, return false.

I came across this in your code, inside your board_limit method:

if len(self.board[board_number - 1]) < 3:
    return True
else:
    return False

This is unnecessary. If the condition in the if statement evaluates to true, why don't you just return the conditional itself?

return len(self.board[board_number - 1]) < 3

Refactor methods

Look at your place_first_cards and place_cards methods. Then, put them in a diff tool. What's the different? Here:

while len(self.hand) != 0:

and:

self.hand.pop(0)

These are the only two lines that are different among these two methods. Everything else is exactly the same.

You should refactor this into one method that takes a parameter that tells if the cards are the first cards or not. Then, your new method would look like this:

def place_cards(self, is_first):
    print "Your time to play %s. Here is your hand. Pick a card and choose a row to place it" % self.name
    while len(self.hand) != (0 if is_first else 1): <--------
        print '-'.join(self.hand)
        for i in self.board:
            print i
        card_placed = int(raw_input("Choose a card (1 - %d): " % len(self.hand)))
        chosen_board = int(raw_input("Choose a board (1-3): "))
        if self.board_limit(chosen_board):
            self.board[chosen_board - 1].append(self.hand[card_placed - 1])
            self.hand.pop(card_placed - 1)
        else:
            print "The board is full. Choose another one."
    if not is_first:
        self.hand.pop(0) <------

This single method does exactly what those other two methods did, except for two small sections that can be "turned on or off" with a simple argument.

Now, your code would look like this for placing first cards:

user.place_cards(is_first=True)

Note: the is_first= is not necessary, but it increases readability, I think.

Now your code is simpler.


\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. Lots of great insights and info that I didn't know. \$\endgroup\$ – mschlindwein Feb 19 '16 at 12:38
1
\$\begingroup\$

You should try to reduce whitespace and also put brackets on prints.

e.g

print("The board is full choose another one")
\$\endgroup\$
  • 1
    \$\begingroup\$ I thought that would work just for python 3 \$\endgroup\$ – mschlindwein Feb 19 '16 at 12:38
  • \$\begingroup\$ Not that I know of, I may be wrong however. \$\endgroup\$ – D.C Feb 19 '16 at 12:39

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.